Webrev for making parts of TypeAnnotationPosition immutable

[Werner Dietl]

Hi Eric, all,

thanks for these changes!

I adapted the Checker Framework to these revisions and have some comments:

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.


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.


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.


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.

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.


> * 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.

Cheers,
cu, WMD.

On Fri, Jan 31, 2014 at 12:44 PM, Eric McCorkle
<eric.mccorkle at oracle.com> wrote:
> The following webrev implements a number of changes to
> TypeAnnotationPosition.  It makes most of the core data immutable, and
> changes the way that TypeAnnotationPositions are created.
>
> As opposed to the zero-argument constructor with mutable fields,
> TypeAnnotationPositions are now created using static methods.
>
> This patch also marks some field and methods as deprecated (in
> comments), as I plan to remove them in forthcoming patches.
>
> One possibility that came up in review was making TypeAnnotationPosition
> an abstract superclass, and having subclasses for each of the different
> variants.
>
> I also could not make some of the fields final (type_index,
> exception_index, offset), as they are modified by Gen.  This is most
> visible in the updated ClassReader.  I will be investigating ways to
> deal with this in forthcoming patches.
>
> The webrev is here:
> http://cr.openjdk.java.net/~emc/8033004/webrev/
>
> This patch has already been reviewed by the javac team; however, I would
> like to give a chance for any tools (such as checkers), that currently
> use type annotations to run tests before I integrate it.  Please look
> over the patch, and run any tests that should be run.
>
> Thanks,
> Eric



-- 
http://www.google.com/profiles/wdietl