RFR; JDK-8214031: Assertion error in value break statement with conditional operator in switch expression (original) (raw)

Jan Lahoda jan.lahoda at oracle.com
Fri Nov 30 17:11:52 UTC 2018

Hi,

A patch updated according to the comments is here: http://cr.openjdk.java.net/~jlahoda/8214031/webrev.01/

A delta patch from the previous patch is here: http://cr.openjdk.java.net/~jlahoda/8214031/webrev.delta.00.01/

How does this look?

Thanks, Jan

On 29.11.2018 13:42, Jan Lahoda wrote:

Hi Maurizio,

Thanks for the comments. I'll work on these. Jan On 28.11.2018 13:12, Maurizio Cimadamore wrote: Hi Jan, the patch looks very good and, albeit complex, I don't think there's another way to implement shortcircuiting of boolean switch expression w/o going deep in Gen (which is what this patch does). So well done.

From a code perspective I have only minor comments, as the code is generally well done: 1) In Flow, since now you are making AssignAnalyzer also extends from BaseAnalyzer (used to be BaseAnalyzer), I think there is no need now for having a type parameter in BaseAnalyzer. Let all exits be PendingExit - then subclasses can come up with variants (like AssignExit) when needed. But it seems like the 1-1 relationship between analyzer type and exit type is gone and the code should probably be more upfront about that. 2) The valueBreakHandler abstraction in Gen is nice, but I wonder if the code would be clearer by encoding the desired logic directly in visitBreak. That is, visitBreak could fetch the switch expression it points to, and determine whether we are inside a 'cond' or not, and then act accordingly. I think that, with a couple of Gen instance fields (shared trueChain/falseChain), you could be able to get there. Not 100% sure it will make the code better, but I thought it could be worth a try. 3) In the test DefiniteAssignment1.java (and also, but to lesser extent, DefiniteAssignment1.java) I guess it would also be nice to test some && condition in the break expression - e.g. true && (x = 1) Also, might be worth replacing the true with some other variable which the compiler doesn't know to be true statically (as that affects code generation). Cheers Maurizio On 27/11/2018 20:21, Jan Lahoda wrote: Hi,

javac does not handle switch expressions that return booleans correctly. According to the spec, javac should track definite [un]assignment when true/false for switch expressions, but fails to do so. Moreover, for more complex code, like: int v = ...; int x; boolean b = switch (v) { case 0: x = 12; break true; case 1: x = 13; break true; default: break false; } && x == 12; The current switch expression desugaring is not powerful enough to generate valid bytecode for this input. The issue is that the current desugaring will translate the switch expression into code like: ( boolean $result; switch (v) { case 0: x = 12; $result = true; break; case 1: x = 13; $result = true; break; default: $result = false; break; } $result //"return" expression ) && x == 12 But by assigning the true/false result to a variable, we are loosing track of which variables are assigned (when true/false) and which are not. The bytecode needs to ensure the "x == 12" is jumped over when the switch expression's value is false, and that, I believe, is not possible when desugaring in Lower. So, the proposed fix is to avoid desugaring in Lower, letting switch expressions go to Gen and produce bytecode for them, including handling them in Gen.genCond for true/false switch expressions. This is mostly modelled after the conditional expression handling. Some generalizations in Lower are required as well, to handle String and enum switch expressions. Handling of definite assignment in Flow is fixed as well. JBS: https://bugs.openjdk.java.net/browse/JDK-8214031 Webrev: http://cr.openjdk.java.net/~jlahoda/8214031/webrev.00/ Any feedback is welcome! Thanks, Jan

More information about the compiler-dev mailing list