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

Fixed miscellaneous javac compiler warnings #915

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahelwer
Copy link
Contributor

@ahelwer ahelwer commented Apr 24, 2024

These are generally trivial. I blanket-silenced the sun.misc.Unsafe warnings using a javac parameter. I would like to add -Werror to prevent further warnings from being introduced but am blocked on #921

@ahelwer ahelwer changed the title Fixed miscellaneous warnings Fixed miscellaneous javac compiler warnings Apr 24, 2024
Copy link
Collaborator

@Calvin-L Calvin-L left a 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.

@lemmy lemmy added enhancement Lets change things for the better Tools The command line tools - TLC, SANY, ... labels Apr 25, 2024
@ahelwer ahelwer force-pushed the fix-warnings branch 2 times, most recently from d5e2407 to 7ec3b1f Compare April 30, 2024 14:57
Signed-off-by: Andrew Helwer <2n8rn1w1f@mozmail.com>
@ahelwer
Copy link
Contributor Author

ahelwer commented May 2, 2024

@lemmy @Calvin-L any other reviews? I think this can be merged.

<!-- 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" />
Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Contributor Author

@ahelwer ahelwer May 20, 2024

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.

Copy link
Member

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.

@ahelwer
Copy link
Contributor Author

ahelwer commented May 21, 2024

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.

@lemmy
Copy link
Member

lemmy commented May 21, 2024

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. :-(

@Hi-Angel
Copy link
Contributor

Hi-Angel commented May 30, 2024

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.

@ahelwer
Copy link
Contributor Author

ahelwer commented May 30, 2024

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.

@lemmy
Copy link
Member

lemmy commented May 30, 2024

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 -->
Copy link
Member

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.

@Hi-Angel
Copy link
Contributor

This PR is not time-critical.

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.

@lemmy
Copy link
Member

lemmy commented May 30, 2024

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.

@Hi-Angel
Copy link
Contributor

Hi-Angel commented May 30, 2024

Before accusing me of delaying improvements, please reread my previous comments

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:

  1. You said the project is understaffed
  2. I found a way to improve on that
  3. You opposed by saying the features that are delayed are non-critical (so is it understaffed or not? 😊)
  4. I agreed, but pointed out that there is more to it
  5. You commented like I was accusing someone of something

, including the one I made after Andrew's talk at TLA+ Conf 24.

Sorry, I don't know where to find it. Is it important for the context?

Do not treat maintainers as free labor.

Of course not! 😊 If anything, my suggestions are about decreasing the maintainer's load

@lemmy
Copy link
Member

lemmy commented May 30, 2024

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.

@Hi-Angel
Copy link
Contributor

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".

@ahelwer
Copy link
Contributor Author

ahelwer commented May 30, 2024

@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

@lemmy
Copy link
Member

lemmy commented May 30, 2024

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.

@Hi-Angel
Copy link
Contributor

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).

@Hi-Angel
Copy link
Contributor

@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

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:

Screenshot_20240530_214618

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.

@ahelwer
Copy link
Contributor Author

ahelwer commented May 30, 2024

@Hi-Angel it is at 11 am ET and 8 am PT!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Lets change things for the better Tools The command line tools - TLC, SANY, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants