Webrev for making parts of TypeAnnotationPosition immutable

[Eric McCorkle]

On 02/05/14 17:57, Werner Dietl wrote:
> 1. In the type-annotations repository, I added
> 
> com.sun.tools.javac.code.TypeAnnotationPosition.copy(TypeAnnotationPosition)
> 
> This makes it a lot easier to create a new type annotation position
> from an existing one. I previously had this in a Checker Framework
> utility class, but can't implement it there without constructor
> access.

That's probably a worthwhile addition.  I will probably add it in a
later patch (see comments on 2)

> 2. In general TypeAnnotationPosition had all the builder methods that we needed.
> The following are however uglier than I would like:
> 
>   tapos = TypeAnnotationPosition.methodThrows(TypeAnnotationPosition.emptyPath,
> null, tidx, thr.pos);
>   tapos = TypeAnnotationPosition.typeParameter(TypeAnnotationPosition.emptyPath,
> null, tpidx, ((JCTree)tp).pos);
>   tapos = TypeAnnotationPosition.methodTypeParameter(TypeAnnotationPosition.emptyPath,
> null, tpidx, ((JCTree)tp).pos);
>   tapos = TypeAnnotationPosition.typeParameterBound(TypeAnnotationPosition.emptyPath,
> null, tpidx, bndidx, ((JCTree)tp).pos);
>   tapos = TypeAnnotationPosition.methodTypeParameterBound(TypeAnnotationPosition.emptyPath,
> null, tpidx, bndidx, ((JCTree)tp).pos);
> 
> For these it would be nice to have versions that don't take the
> location path and onLambda parameters; such versions exist for the
> other methods.

There is a good chance these will get added.  I created these static
constructors "on demand", and I am currently redoing how
TypeAnnotationPositions get created and assigned to
Attribute.TypeCompounds, so I may end up adding these as well.

If not, I can circle back and add them at the end.

I will probably also add the aforementioned "copy", as I could see where
it would be needed as well.

> 3. In TypeAnnotationPositions, I find the following comment:
> 
>     // These methods are the new preferred way to create
>     // TypeAnnotationPositions
> 
> misplaced. It's before the private constructor instead of before the
> methods the comment is referring to.

Thank you for finding that.  I think I moved the private constructor to
be closer to the static constructors, and must have missed the comment.

> Thanks for sharing all the information from your "Updated type
> annotations roadmap" message.
> I'm very much looking forward to all these improvements!
> 
>> * I will indicate whenever I post a webrev for a major change, to allow
>> the checkers framework tests and other test to be run, thereby ensuring
>> that these patches don't introduce any errors.  (It is, of course, our
> 
> It would be great if there were more time than just a weekend between
> the announcement and the push.

That would be ideal, I agree.  However, we are trying to move these
patches along quickly, as there are other engineering efforts that
depend on them.  The logistics of trying to maintain my ongoing
development efforts, while there are large patches that haven't gone in
yet are a bit difficult, I'm afraid.

> 
> Are you running the Checker Framework as part of your test suite? We
> had discussed this previously, but I'm not sure whether it was ever
> integrated.
> I will of course adapt the Checker Framework if there are any
> interface changes like with this change.
> However, for changes where you do not expect any impact on tools, it
> would be easiest if you ran the Checker Framework test suite. It is a
> much more comprehensive set of tests.
> 

We discussed this amongst the javac team, actually.  It's probably best
to wait until the end of the roadmap before importing any changes and
running Checkers.  The reason is that I am trying to make these changes
as incremental as possible, but in reality they are all steps in a
rather significant change, and I think there would be a lot of needless
effort to try to keep checkers in sync with all the intermediate states.

Also, my plan at the end of this is to combine all the patches into a
single bulk patch, which will be applied at some point to the 8u repo.
So if we want to put these changes back into the type annotations repo,
it's probably best to wait until I produce that bulk patch.

> 
>> * There should be a final merge of the type annotations repository with
>> the jdk9 langtools repository.  Following that, I propose discontinuing
>> the type annotations repo.  Any changes to type annotations after that
> 
> I'm sure you're aware of this thread were we discussed some outstanding changes:
> 
> http://mail.openjdk.java.net/pipermail/type-annotations-dev/2014-January/001533.html
> http://mail.openjdk.java.net/pipermail/type-annotations-dev/2014-January/001543.html
> 
> I just performed a sync of type-annotations and jdk9/dev and these
> differences still exist.
> 

Thank you for posting links to those.  Going just on file names, those
don't seem to be related, but I'd need to look at them in more detail.

I am not sure the patches can be applied as-is, but I will make sure the
changes are reflected at the end of the current effort.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 303 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/type-annotations-dev/attachments/20140206/6ad9d6a3/eric_mccorkle.vcf 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
Url : http://mail.openjdk.java.net/pipermail/type-annotations-dev/attachments/20140206/6ad9d6a3/signature.asc