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

Fail Maven build on compiler warnings; remove some warning suppressions #2183

Merged
merged 6 commits into from
Aug 27, 2022

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Aug 22, 2022

Currently this enables all compiler warnings (-Xlint:all); I am not sure if that is a good idea. If you want we can also restrict it to specific warnings instead. For example warnings for serial seem to be redundant here and varargs seems to not be very helpful. I am also a bit afraid that the supported and detected warnings depend on the JDK version being used (instead of the --release version being specified).

It also looks like javac and Eclipse IDE disagree regarding when a warning needs to be suppressed. For some of the required suppressions here Eclipse is telling me "Unnecessary @ SuppressWarnings".

Supersedes #1374

Please let me know what you think.

@Marcono1234
Copy link
Collaborator Author

I am also a bit afraid that the supported and detected warnings depend on the JDK version being used (instead of the --release version being specified).

Yes, that can be seen with the failed CIFuzz run:

Warning:  COMPILATION WARNING : 
[INFO] -------------------------------------------------------------
Warning:  source value 7 is obsolete and will be removed in a future release
Warning:  target value 7 is obsolete and will be removed in a future release
Warning: [WARNING] To suppress warnings about obsolete options, use -Xlint:-options.
[INFO] 3 warnings 
[INFO] -------------------------------------------------------------
[INFO] -------------------------------------------------------------
Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  warnings found and -Werror specified

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

I'm OK with this if you can fix the CIFuzz problem. I suspect we may find that failOnWarning is more trouble than it's worth but it won't be hard to back just that part out if so.

@@ -126,10 +126,10 @@ private ReflectiveTypeAdapterFactory.BoundField createBoundField(
final TypeToken<?> fieldType, boolean serialize, boolean deserialize,
final boolean blockInaccessible) {
final boolean isPrimitive = Primitives.isPrimitive(fieldType.getRawType());
// special casing primitives here saves ~5% on Android...
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Aug 26, 2022

Choose a reason for hiding this comment

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

Have removed this comment because it is a bit misleading (at least at the current line where this is placed). It appears this was added in 2011 after experiments with Field.setInt and similar. Though I am not sure if that code is part of the Git history. Additionally since currently Gson only supports Object values, primitives would always be boxed. It would also have to be verified if these 5% difference still applies after now 11 years.

@Marcono1234
Copy link
Collaborator Author

Have excluded the options warnings which were causing the failure for CIFuzz, and have also tried to improve the placement for some @SuppressWarnings annotations. I hope that is alright.

Though I agree, if failOnWarning is causing too much issues we could remove it again.

@eamonnmcmanus
Copy link
Member

This is great, thanks!

@eamonnmcmanus eamonnmcmanus merged commit f7a164d into google:master Aug 27, 2022
@Marcono1234 Marcono1234 deleted the marcono1234/javac-warnings branch August 27, 2022 01:35
@Marcono1234 Marcono1234 mentioned this pull request Aug 27, 2022
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.

2 participants