-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
Fixed miscellaneous javac compiler warnings #915
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. If anything you've understated the importance of this change; new Integer
and new Boolean
are terminally deprecated as of Java 16 (released more than 3 years ago). Removing uses of those constructors is something we need to do for longevity of the project.
d5e2407
to
7ec3b1f
Compare
Signed-off-by: Andrew Helwer <2n8rn1w1f@mozmail.com>
<!-- compilerarg value="-Xlint:deprecation"/--> | ||
<!-- Suppress sun.misc.Unsafe warnings because Java can't allocate an | ||
array with more than 2 billion elements. --> | ||
<compilerarg line="-XDignore.symbol.file=src/tlc2/tool/fp/LongArray.java" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me thinks that the warnings about Unsafe
are good, actually, and should be retained. But I think that adding -Werror
is a noble goal, and what you've done here is probably the simplest solution in pursuit of that goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment about 2b elements is only part of the story. That could also be worked around by using an array of arrays like in MSBFPSet. Using Unsafe also has the advantage that it moves the array out of the "control" of the garbage collector that won't be able to ever reclaim any memory anyway.
The reason why it might make sense to migrate to the official successor API in recent JVMs is also direct support to allocates zeroed memory with calloc
(currently a bottleneck in our LongArray). I once wrote a JVM patch that exposed calloc in Unsafe. I went as far as proposing it on the valhalla mailinglist, but the timing for its inclusion wasn't right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like keeping the warnings around because they're good warnings, and even actionable if we can upgrade to the new API we talked about in the community chat last week, but they also make it difficult to find actual compiler errors in the text that is emitted. We could create a new issue to track solutions to these warnings, although I don't know how valuable the issue tracker is as a to-do list these days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please amend the comment to mention that LongArray also doesn't incur GC cost.
On a meta note how can we speed up development processes? I am not trying to be critical here. But these are very simple changes and this PR was opened a month ago. There is no path to improving the codebase if PRs like these take one month to resolve. |
The project has been understaffed for a long time. There are two active committers who can review and merge PRs now, but TLC maintenance is not our day job. Given this bandwidth constraint, the pursuit of noble goals is difficult when there are fires elsewhere. :-( |
@lemmy since @ahelwer has 26 PRs to the project at this point, is that possible perhaps to give @ahelwer write permissions to the project? That wouldn't oblige @ahelwer to anything, but it may make it easier to improve the project further. In my experience that's what people do when there's not enough maintainers but there are motivated contributors. For example, I was given permissions to color-identifiers-mode because I've been contributing, so, like, "why not". On the opposite side, I was also given "Developer" permissions to libinput because I had contributed to it, but it just so happened for unrelated reasons that I almost stopped contributing around that point of time. This is an example that @ahelwer isn't obliged to do anything. It's just a path to potential improvements, no more, no less. |
We could do something like if I submit a PR, there is a two week period where people can ask for more time to review it if they see anything objectionable at first glance. But if nobody explicitly blocks the PR within two weeks, it gets merged - something like that. |
This PR is not time-critical. The only changes that are time-critical are those that address known or potential completeness or soundness bugs. Moreover, the community can always step up and provide thorough PR reviews, as seen in #907. |
= OffHeapDiskFPSet won't work anymore. = | ||
================================================================ | ||
</echo> | ||
<!-- compile --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message of this warning remains valid even if warnings are disabled. It should not be removed but reworded to inform users if compilation fails due to an upcoming JVM update that removes Unsafe.
True, but misses the point. Improvements are usually based upon other improvements (even when improvements are unrelated, I personally prefer to wait for existing changes to get merged just to see that my work wasn't in vain). Regularly delaying "non-critical changes" would result in stopping improvements from happening and basically project stagnation and bit-rot. I'm not a Java developer, so I can't say much about the code, but looking at some problems, various @ahelwer's fixes, and seeing how at least one bug is due to an ancient version of a library used, the project does seem to be in need of refactoring. |
Before accusing me of delaying improvements, please reread my previous comments, including the one I made after Andrew's talk at TLA+ Conf 24. Do not treat maintainers as free labor. |
You probably misunderstand me if you think there's accusation of any sort I did just re-read the your comments, there's not many of them. Let me reiterate the current discussion, just to make sure we are all on the same page:
Sorry, I don't know where to find it. Is it important for the context?
Of course not! 😊 If anything, my suggestions are about decreasing the maintainer's load |
You can easily find the recording online, but to summarize: this is an open-source project. Open source allows the community to propose changes (issues, PRs, etc.). However, it does not obligate the project maintainers to accept or reject contributions. Some contributions may even be ignored due to resource limitations or cost-benefit considerations. If groups within the community disagree with the maintainers' decisions because their interests don't align with the project's agenda, they can fork the project and pursue their own agenda. Am I suggesting that you fork the project? No, as evidenced by my review of past PRs related to Unicode support and code refactorings, which do not even align with my employer's agenda. However, I won't commit to any SLAs. Reviews will continue to happen in due time or not at all. |
Oh yeah, I'm sure we're all agree on that. It's just that the comment per se is a bit orthogonal to the discussion (unless I'm missing something)… But okay, let's assume this implies the answer is "no, we're not interested in maintenance help". |
@Hi-Angel we can certainly all discuss this on video chat at the next community call: https://calendar.google.com/calendar/embed?src=cb3f93f188c92378a8fec42b25365ab2a64665d770a8265c1fcec00e03823c6c%40group.calendar.google.com&ctz=America%2FNipigon |
It is a big leap to go from nominating one contributor who doesn't get support from one maintainer to concluding that the project is generally not interested in maintenance help. |
I'll refrain from commenting on the issue any further because there's clearly some confusion going on and continuing is pointless. Because your every comment is, like… I don't know, doesn't take the full picture, perhaps… Like, it kind of related to the topic, but at the same time it isn't. It might be due to us communicating via text, I've seen this sometimes happening. @ahelwer's idea about the video call is a good one I think (I am completely new to TLA+ so I didn't even know these events are happening). I'll see if I can make it to the meeting (but I won't promise). |
I am sorry for stupid question, but… what timezone is it in? 😅 I've been messing with it for around 10 minutes and got 2 different answers from Google Calendar: Yes, this is the same event, which Google Calendar claims to be happening in two different unrelated times. I think I saw somewhere mention of GMT-7, which implies 18:00 in my timezone to be closer to reality… But I can't find that note anymore so am not sure. |
@Hi-Angel it is at 11 am ET and 8 am PT! |
These are generally trivial. I blanket-silenced the
sun.misc.Unsafe
warnings using ajavac
parameter. I would like to add-Werror
to prevent further warnings from being introduced but am blocked on #921