<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Looks great.</p>
<p>One minor nit that I had not notice yesterday on the error
messages; I think this message:</p>
<pre><span class="new">switch expression completes without providing a value\n\</span>
<span class="new">+ (switch expressions must either provide a value or throw for all possible input values)</span></pre>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Works better than this</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">
<pre><span class="new">switch rule completes normally\n\</span>
<span class="new">+ (switch rules in switch expressions must either provide a value or throw)</span></pre>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">The reason being that "switch rule
completes normally", the first line of the message, is kind of
vague, and does not give many indications of what's wrong.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">I'd prefer something like this:</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">
<div class="moz-cite-prefix">
<pre><span class="new">switch rule completes without providing a value\n\</span>
<span class="new">+ (switch rules in switch expressions must either provide a value or throw)</span></pre>
</div>
<div class="moz-cite-prefix"><br>
</div>
</div>
<div class="moz-cite-prefix">There is a bit of repetition, yes, but
I think it's a better message.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">I'll leave this to your consideration.
No need for another review.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Thanks!<br>
Maurizio<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 21/11/2018 10:31, Jan Lahoda wrote:<br>
</div>
<blockquote type="cite" cite="mid:5BF533F1.5050605@oracle.com">Hi
Maurizio,
<br>
<br>
Thanks for the comments. I've renamed NOT_ALIVE to DEAD, as
suggested, and replace "alive.alive" with appropriate tests
(usually either == Liveness.DEAD or != Liveness.DEAD). I was first
afraid this would look too bad, but seems to be quite reasonable
to me in the end.
<br>
<br>
Updated webrev is here:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jlahoda/8212982/webrev.01/">http://cr.openjdk.java.net/~jlahoda/8212982/webrev.01/</a>
<br>
<br>
Any further feedback is welcome!
<br>
<br>
Thanks,
<br>
   Jan
<br>
<br>
On 20.11.2018 19:17, Maurizio Cimadamore wrote:
<br>
<blockquote type="cite">I like the tri-state fix; there is however
something that leaves me
<br>
perplexed. I see often idioms such as
<br>
<br>
if (alive.alive ...
<br>
<br>
Now, it seems like the Liveliness enum has a dual nature; on the
one
<br>
hand it's a tri-state (alive, dead, recovery). But there are
cases in
<br>
which you want to project it down to a two-state. The use of an
hidden
<br>
boolean here makes the code less readable, because a reader
might ask -
<br>
how is that different from alive == Liveliness.ALIVE?
<br>
<br>
I think we should try to think as to what is the condition that
we want
<br>
to capture with the alive.alive and then have a method on the
enum which
<br>
returns 'true' for certain constants and false for others.
<br>
<br>
I think what the code is doing is checking whether the
liveliness *is
<br>
not* dead.
<br>
<br>
So perhaps, conditions such as alive.alive might better be
rewritten as
<br>
alive != Liveliness.NOT_ALIVE
<br>
<br>
(btw, probably better to call NOT_ALIVE just DEAD and avoid all
the
<br>
double negations).
<br>
<br>
Maurizio
<br>
<br>
<br>
On 20/11/2018 17:14, Jan Lahoda wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8212982">https://bugs.openjdk.java.net/browse/JDK-8212982</a>
<br>
<br>
The issue here is that javac accepts switch expressions that
complete
<br>
normally without providing a value, which is not correct. A
(simpler)
<br>
fix is to enhance Flow with the necessary checks +
enhancements to
<br>
place the errors at a good place.
<br>
<br>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jlahoda/8212982/webrev.00/">http://cr.openjdk.java.net/~jlahoda/8212982/webrev.00/</a>
<br>
<br>
This patch has a problem in cases like:
<br>
---
<br>
public class SE {
<br>
    private String t(int i) {
<br>
        return switch (i) {
<br>
            default:
<br>
                break "";
<br>
                System.err.println(0);
<br>
        };
<br>
    }
<br>
}
<br>
---
<br>
<br>
This produces:
<br>
---
<br>
$ javac --enable-preview --source 12 SE.java
<br>
SE.java:6: error: unreachable statement
<br>
                System.err.println(0);
<br>
                ^
<br>
SE.java:7: error: switch expression completes without
providing a value
<br>
        };
<br>
        ^
<br>
 (switch expressions must either provide a value or throw for
all
<br>
possible input values)
<br>
---
<br>
<br>
The second error is caused by the first one (Flow will reset
"alive"
<br>
to "true" when reporting the "unreachable statement" error as
an error
<br>
recovery). A patch that changes the "alive" field to be
tri-state
<br>
(ALIVE, NOT_ALIVE, RECOVERY) so that it can suppress the
second error
<br>
in case of this recovery is here:
<br>
<br>
Webrev 2:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jlahoda/8212982/webrev.00b/">http://cr.openjdk.java.net/~jlahoda/8212982/webrev.00b/</a>
<br>
<br>
(The only differences between the patches are in the Flow.java
and
<br>
ExpressionSwitchUnreachable.out.)
<br>
<br>
This is longer, but I think it provides better errors, so I'd
prefer
<br>
this patch, but I am also fine with the first one.
<br>
<br>
Any feedback is welcome!
<br>
<br>
Thanks,
<br>
   Jan
<br>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>