Type annotations on inner type that is an array components

[B. Blaser]

I attached one more iteration (d) of the fix without any side-effect
which is closer to 'jdk8208470b.patch' based on your second fix + the
latest points we discussed.
The issue is that resetting or not the type of the tree in
'typeWithAnnotations()' has some side-effects which is revealed by
'T6747671.out' but I'm not sure what's really expected?

So, both (b) and (d) variants seem good to me.
Variant (c) is more conservative but I'm not sure this is really
necessary (the logic I kept was added as part of the jdk9 change-set).

Any feedback is welcome,
Bernard


On 30 July 2018 at 23:59, B. Blaser <bsrbnd at gmail.com> wrote:
> On 30 July 2018 at 20:19, Werner Dietl <wdietl at gmail.com> wrote:
>> In this part:
>>
>>              // Update first position
>>              for (TypeCompound tc : annotations) {
>>                  if (tc.position == null)
>>                      tc.position = pos;
>>                 tc.position.location =
>> tc.position.location.prepend(TypePathEntry.ARRAY);
>>
>> I think you can simplify to:
>>
>>     TypeCompound tc = annotations.get(0);
>>     tc.position = pos;
>>     tc.position.location = tc.position.location.prepend(TypePathEntry.ARRAY);
>>
>> You know that annotations is non-empty, so there is always at least
>> the first element that you need to update.
>> I'm not sure why the null check was there. Even if the position is
>> already non-null, don't we want to set it to the position we
>> determined?
>
> I wasn't sure of this, that's why I conservatively kept this check.
> But I agree that 'tc = annotations.get(0)' would be possible too.
>
>> Should we change `arrayTypeTree` to `arrayTypeTreeElement` or some
>> such and put the `.elemtype` access in the helper method?
>
> Why not but does this really improve readability?
>
>> The change to T6747671.out seems ok to me - the error message now
>> contains the annotation on the type use.
>
> I've attached a variant of the fix without any side-effect (the new
> elements discussed above are not yet included but I reinstated the
> dead code removal).
> Any comment about this variant (and 'T6747671' expected output) is welcome.
>
>> Thanks for adding the test cases!
>
> Thank you too,
> Bernard
>
>> Best,
>> cu, WMD.
>>
>>
>>
>>
>>
>> --
>> http://www.google.com/profiles/wdietl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jdk8208470d.patch
Type: text/x-patch
Size: 8685 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180731/425c22cb/jdk8208470d.patch>