Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warnings #596

Merged
merged 7 commits into from
Oct 7, 2019
Merged

Fix warnings #596

merged 7 commits into from
Oct 7, 2019

Conversation

oswetto
Copy link
Contributor

@oswetto oswetto commented Aug 27, 2019

No description provided.

@gbrail
Copy link
Collaborator

gbrail commented Aug 30, 2019

Thanks for working on improving the codebase!

It looks like this PR mainly adds SerialVersionUIDs where they're missing. However, for backward compatibility and general goodness, I think we should follow the existing pattern in the codebase of including the generated UID (as generated by "serialver" or your IDE) rather than setting them all to 1.

Also it looks like the Checkstyle checks fail in the Jenkins build. You can check these yourself by running "./gradlew check".

Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks basically fine, although I see some public methods removed from "NativeError". I'd prefer if this change stuck to formatting and serialVersionUID stuff, and furthermore I'm not sure why we'd want to remove those.

Also, I see a lot of test failures in Jenkins... does running "./gradlew check" work for you?

src/org/mozilla/javascript/NativeError.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making progress and thanks for sticking with it! However if we're going to change serialVersionUIDs we have to be very careful not to break backward compatibility, in the event that someone is relying on this stuff.

So... if a class has no serial version at all, then it makes sense to add one, and adding it by running "serialver" as you did is the correct way because it sets it to the current value that Java will generate automatically.

However, if a class already has one, even if it is 1, then changing that will break compatibility for serialized objects.

So in order to guarantee that nothing breaks, then this change should add serialVersionUID using serialver if they don't exist at all, which is what you have done. However, it should not change any existing ones even if they are set to 1.

Thanks!

Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost! But I count three more "-1s" that we still should not remove.

src/org/mozilla/javascript/LazilyLoadedCtor.java Outdated Show resolved Hide resolved
src/org/mozilla/javascript/NativeArrayIterator.java Outdated Show resolved Hide resolved
src/org/mozilla/javascript/NativeWith.java Show resolved Hide resolved
@gbrail
Copy link
Collaborator

gbrail commented Oct 7, 2019

Now that looks right. Thanks!

@gbrail gbrail merged commit c214075 into mozilla:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants