On 5/23/19 6:39 AM, Langer, Christoph wrote:
big thanks for your great updates here. This all looks a lot cleaner:
http://cr.openjdk.java.net/~clanger/webrevs/8223553.3/
Great, glad to help. While I'm still unsure about the underlying reasons for
this disagreement between the compilers, that might take a while to resolve.
Putting in these fixes isn't very intrusive and it seems to make people's lives
easier, so I don't have any objection to them going in.
In the other 3 locations, we're able to eliminate the EC4J issues by
introducing a local variable with the right type declaration. Sounds like a
viable workaround.
At Eclipse we have the following bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=530236. It refers to the OpenJDK
bug https://bugs.openjdk.java.net/browse/JDK-8016207.
I'm wondering whether this should be submitted and I should at the same time
submit another bug to evaluate these code places at a time when the final
resolution for JDK-8016207 was provided?
Overall, introducing a local variable is more-or-less reasonable even if it's
used only once. One point is that somebody might come along and "clean up" the
code and inline local variables and reintroduce the problem. Another point is
that, hypothetically, if in the future Eclipse is changed to match javac's
behavior, these changes should be reverted.
The bug database is a good place to capture actions that need to take place in
the future. Unfortunately, it's pretty far from these locations, so the
existence of such a bug wouldn't prevent the accidental cleanup from happening.
That would indicate having a comment in the code at these locations.
On the other hand, if Eclipse is "fixed" and these locations don't get cleaned
up for a long time, it doesn't seem like that big a deal.
I'd suggest to put a comment at these 3 locations, something like:
// local variable required here; see JDK-8223553
and not bother with filing another bug report to track this.
I'll defer to Martin as to how he wants to handle the ConcurrentSkipListMap.java
change.
s'marks
