RFR 8221118: Avoid eagerly creating JCDiagnostic for CompletionFailures

[Ron Shapiro]

Sure thing. Here's that change:
http://cr.openjdk.java.net/~ronsh/8221118/webrev.05/

Thanks for the review!

On Fri, Mar 29, 2019 at 7:40 AM Maurizio Cimadamore <
maurizio.cimadamore at oracle.com> wrote:

> Looks good - for bonus points, you could condense the code a bit - e.g.
> turn this
>
> Supplier<JCDiagnostic> diag =
> +                () ->
> diagFactory.fragment(Fragments.ClassFileNotFound(c.flatname));
>              return newCompletionFailure(c, diag);
>
> into this:
>
>              return newCompletionFailure(c, () ->
> diagFactory.fragment(Fragments.ClassFileNotFound(c.flatname)));
>
>
> When the local var type used to be JCDiagnostic it was sort of ok, now we
> have all these Supplier<JCDiagnostic> plus a throwaway variable name...
>
> I'll leave this to your judgement.
>
> Maurizio
> On 28/03/2019 22:42, forax at univ-mlv.fr wrote:
>
> Looks good to me.
>
> Rémi
>
> ------------------------------
>
> *De: *"Ron Shapiro" <ronshapiro at google.com> <ronshapiro at google.com>
> *À: *"Remi Forax" <forax at univ-mlv.fr> <forax at univ-mlv.fr>
> *Cc: *"jonathan gibbons" <jonathan.gibbons at oracle.com>
> <jonathan.gibbons at oracle.com>, "compiler-dev"
> <compiler-dev at openjdk.java.net> <compiler-dev at openjdk.java.net>
> *Envoyé: *Jeudi 28 Mars 2019 23:30:25
> *Objet: *Re: RFR 8221118: Avoid eagerly creating JCDiagnostic for
> CompletionFailures
>
> Here's an updated webrev with resetDiagnostic():
> http://cr.openjdk.java.net/~ronsh/8221118/webrev.04/
>
> On Thu, Mar 28, 2019 at 6:14 PM <forax at univ-mlv.fr> wrote:
>
>>
>>
>> ------------------------------
>>
>> *De: *"Ron Shapiro" <ronshapiro at google.com>
>> *À: *"Remi Forax" <forax at univ-mlv.fr>
>> *Cc: *"jonathan gibbons" <jonathan.gibbons at oracle.com>, "compiler-dev" <
>> compiler-dev at openjdk.java.net>
>> *Envoyé: *Jeudi 28 Mars 2019 22:58:00
>> *Objet: *Re: RFR 8221118: Avoid eagerly creating JCDiagnostic for
>> CompletionFailures
>>
>> It's currently being reset by the cached CompletionFailure in ClassFinder
>> - I can replace that with a method call that resets the supplier and then
>> nulls the diagnostic. Suggestions on a method name for this?
>>
>>
>> resetDiagnostic() ?
>>
>> Does this complicate the API of this type?
>>
>>
>> yes, but i believe the complexity comes from the fact you want lazy
>> loading thus it's not a plain data type anymore.
>>
>> Rémi
>>
>>
>> On Thu, Mar 28, 2019 at 5:51 PM Remi Forax <forax at univ-mlv.fr> wrote:
>>
>>> Hi Ron,
>>> In Symbol.java, having the field 'diag' being public means that you can
>>> access to the object without calling the getter so without calling the
>>> supplier.
>>> I also think that the supplier should not be public too.
>>>
>>> regards,
>>> Rémi
>>>
>>> ------------------------------
>>>
>>> *De: *"Ron Shapiro" <ronshapiro at google.com>
>>> *À: *"jonathan gibbons" <jonathan.gibbons at oracle.com>
>>> *Cc: *"compiler-dev" <compiler-dev at openjdk.java.net>
>>> *Envoyé: *Mercredi 27 Mars 2019 18:46:09
>>> *Objet: *Re: RFR 8221118: Avoid eagerly creating JCDiagnostic for
>>> CompletionFailures
>>>
>>> I missed a few cases earlier - see this updated webrev:
>>> http://cr.openjdk.java.net/~ronsh/8221118/webrev.01/
>>>
>>> On Tue, Mar 26, 2019 at 4:10 PM Jonathan Gibbons <
>>> jonathan.gibbons at oracle.com> wrote:
>>>
>>>> Looks OK to me, especially the similarity to JDK-8215368
>>>>
>>>> -- Jon
>>>>
>>>> On 03/19/2019 03:37 PM, Ron Shapiro wrote:
>>>>
>>>> Hi,
>>>>
>>>> This is a small change that avoids eagerly creating JCDiagnostic for
>>>> CompletionFailures.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ronsh/8221118/webrev.00/
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8221118
>>>>
>>>> Thanks,
>>>> Ron
>>>>
>>>>
>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190329/a1026769/attachment.html>