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

Adopt "errorprone" tool instead of Spotbugs #1525

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Conversation

gbrail
Copy link
Collaborator

@gbrail gbrail commented Jul 17, 2024

Compared to spotbugs, errorprone seems to run faster, finds a number of
useful things, is easier to configure, and is updated often.

  • Add errorprone to the build for all non-test code
  • Suppress or disable warnings that are too much for us to fix now
  • Fix other errors and warnings
  • Remove spotbugs (it and errorprone don't always agree)

Other notes:

  • We always specify a character set for string conversions and output writing now. This is hard-coded to UTF-8 which I believe will be the right choice for everyone AFAIK.
  • errorprone is very pedantic about parenthesis, but I think it will make the code more consistent in the long run so I left these warnings enabled
  • errorprone is very unhappy about many aspects of our Javadoc and I decided to disable those checks. Someone else might find it rewarding to enable them and fix the problems that result.

gbrail added 3 commits July 17, 2024 16:25
Compared to spotbugs, errorprone seems to run faster, finds a number of
useful things, is easier to configure, and is updated often.

* Add errorprone to the build for all non-test code
* Suppress or disable warnings that are too much for us to fix now
* Fix other errors and warnings
* Remove spotbugs (it and errorprone don't always agree)
@gbrail
Copy link
Collaborator Author

gbrail commented Jul 18, 2024

This change improves build times a little bit, makes the code more consistent and modern, and found three bugs:

  • A possible ConcurrentModificationException in the Java adapter
  • Some dodgy concurrency practices in two places

Given that, I'm going to go ahead and merge it, although it may require be to do a bit more work on a few of the in-flight PRs.

@gbrail gbrail merged commit 7feaad0 into mozilla:master Jul 18, 2024
3 checks passed
@gbrail gbrail deleted the errorprone branch July 18, 2024 20:41
@@ -667,7 +666,7 @@ private Field[] getAccessibleFields(boolean includeProtected, boolean includePri
for (Field field : declared) {
int mod = field.getModifiers();
if (includePrivate || isPublic(mod) || isProtected(mod)) {
if (!field.isAccessible()) field.setAccessible(true);
field.trySetAccessible();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gbrail is there any reason why we not forward this to the vm bridge?
I guess performance might be no real reason because hopefully the vm inlines the call

@@ -328,8 +328,7 @@ private void discoverAccessibleMethods(
if (isPublic(mods) || isProtected(mods) || includePrivate) {
MethodSignature sig = new MethodSignature(method);
if (!map.containsKey(sig)) {
if (includePrivate && !method.isAccessible())
method.setAccessible(true);
if (includePrivate) method.trySetAccessible();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as below

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