[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-compiler-dev
Subject: Re: RFR: JDK-8236692: static final fields without initializer are accepted by javac
From: Vicente Romero <vicente.romero () oracle ! com>
Date: 2020-01-08 16:11:45
Message-ID: b5757c6d-ba49-79b2-8baa-7c6164f4142f () oracle ! com
[Download RAW message or body]
On 1/8/20 10:15 AM, Maurizio Cimadamore wrote:
>
>
> On 08/01/2020 13:40, Vicente Romero wrote:
>>
>>
>> On 1/8/20 6:38 AM, Maurizio Cimadamore wrote:
>>>
>>> The Flow patch looks great and it brings back some sanity into the
>>> code - after all adding the special case for compact constructor is
>>> not so bad, and is now called out in its own special case, which
>>> makes the code more readable.
>>>
>>> On the test - I'm not sure, why is the test source defined in two
>>> strings - as in
>>>
>>> "record R() { # }", "static final String x;"
>>> Doesn't this mean the second string will be replaced for the "#" ?
>>
>> yes
>>
>>> But since there's only one string, why having the replacement at
>>> all? I noted this idiom in most of the negative tests.
>>
>> yes we can do either, although there are already other cases of one
>> liners using this idiom
>
> To be clear, I'm fine for this changeset, I guess I'm more curious as
> to why the framework is being used in this way - to avoid multiline
> string?
>
it was created around the time we started to use multiline strings. We
can in a separate effort rewrite the tests to use multiline strings as
much as possible
> Maurizio
>
Vicente
>
>>
>>
>>> Maurizio
>>
>> Vicente
>>
>>>
>>> On 08/01/2020 03:39, Vicente Romero wrote:
>>>> Hi,
>>>>
>>>> I have updated the patch after an offline review with Maurizio,
>>>> basically I have rewritten how Flow deals with the compact
>>>> constructor to isolate it as a special case. The new iteration is
>>>> at [1]. I have basically restored the original code for method
>>>> checkInit and added a special case for the compact constructor.
>>>>
>>>> Thanks,
>>>> Vicente
>>>>
>>>> [1] http://cr.openjdk.java.net/~vromero/8236692/webrev.01/
>>>>
>>>> On 1/7/20 3:18 PM, Vicente Romero wrote:
>>>>> Please review the fix for [1] at [2], javac was failing to issue
>>>>> an error for code like:
>>>>>
>>>>> record R() {
>>>>> public static final int X;
>>>>> }
>>>>>
>>>>> even though `X` was not initialized as the automatic code to
>>>>> generate initializer expressions only apply to the fields
>>>>> generated from the record component. The patch updates the code in
>>>>> Flow that analyzes compact constructors to don't bail out if the
>>>>> variable being analyzed is static.
>>>>>
>>>>> Thanks,
>>>>> Vicente
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8236692
>>>>> [2] http://cr.openjdk.java.net/~vromero/8236692/webrev.00/
>>>>
>>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 1/8/20 10:15 AM, Maurizio Cimadamore
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e13bfb07-03d5-2e1f-4180-c63d4f7c6621@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p><br>
</p>
<div class="moz-cite-prefix">On 08/01/2020 13:40, Vicente Romero
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:538b8993-a0b6-f4af-c78e-e06fc84e4494@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<br>
<br>
<div class="moz-cite-prefix">On 1/8/20 6:38 AM, Maurizio
Cimadamore wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6a18c3cd-eb9a-c163-5dec-df47458b367b@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>The Flow patch looks great and it brings back some sanity
into the code - after all adding the special case for
compact constructor is not so bad, and is now called out in
its own special case, which makes the code more readable.</p>
<p>On the test - I'm not sure, why is the test source defined
in two strings - as in</p>
<pre><span class="new">"record R() { # }", "static final String x;"</span></pre>
<div class="moz-cite-prefix">Doesn't this mean the second
string will be replaced for the "#" ? </div>
</blockquote>
<br>
yes <br>
<br>
<blockquote type="cite"
cite="mid:6a18c3cd-eb9a-c163-5dec-df47458b367b@oracle.com">
<div class="moz-cite-prefix">But since there's only one
string, why having the replacement at all? I noted this
idiom in most of the negative tests.</div>
</blockquote>
<br>
yes we can do either, although there are already other cases of
one liners using this idiom<br>
</blockquote>
<p>To be clear, I'm fine for this changeset, I guess I'm more
curious as to why the framework is being used in this way - to
avoid multiline string?</p>
</blockquote>
<br>
it was created around the time we started to use multiline strings.
We can in a separate effort rewrite the tests to use multiline
strings as much as possible<br>
<br>
<blockquote type="cite"
cite="mid:e13bfb07-03d5-2e1f-4180-c63d4f7c6621@oracle.com">
<p>Maurizio<br>
</p>
</blockquote>
<br>
Vicente<br>
<blockquote type="cite"
cite="mid:e13bfb07-03d5-2e1f-4180-c63d4f7c6621@oracle.com">
<p> </p>
<blockquote type="cite"
cite="mid:538b8993-a0b6-f4af-c78e-e06fc84e4494@oracle.com"> <br>
<br>
<blockquote type="cite"
cite="mid:6a18c3cd-eb9a-c163-5dec-df47458b367b@oracle.com">
<div class="moz-cite-prefix"> Maurizio<br>
</div>
</blockquote>
<br>
Vicente<br>
<br>
<blockquote type="cite"
cite="mid:6a18c3cd-eb9a-c163-5dec-df47458b367b@oracle.com">
<div class="moz-cite-prefix"> </div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 08/01/2020 03:39, Vicente
Romero wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2116375e-4a3b-5b88-5560-c1d9d31c18e3@oracle.com">Hi,
<br>
<br>
I have updated the patch after an offline review with
Maurizio, basically I have rewritten how Flow deals with the
compact constructor to isolate it as a special case. The new
iteration is at [1]. I have basically restored the original
code for method checkInit and added a special case for the
compact constructor. <br>
<br>
Thanks, <br>
Vicente <br>
<br>
[1] <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~vromero/8236692/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~vromero/8236692/webrev.01/</a>
<br>
<br>
On 1/7/20 3:18 PM, Vicente Romero wrote: <br>
<blockquote type="cite">Please review the fix for [1] at
[2], javac was failing to issue an error for code like: <br>
<br>
record R() { <br>
public static final int X; <br>
} <br>
<br>
even though `X` was not initialized as the automatic code
to generate initializer expressions only apply to the
fields generated from the record component. The patch
updates the code in Flow that analyzes compact
constructors to don't bail out if the variable being
analyzed is static. <br>
<br>
Thanks, <br>
Vicente <br>
<br>
[1] <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8236692"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8236692</a>
<br>
[2] <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~vromero/8236692/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~vromero/8236692/webrev.00/</a>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic