-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add aliasing annotations #57
Add aliasing annotations #57
Conversation
Please add a link to the corresponding checker-framework PR that removes that stub file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also pull in master to resolve the merge issues.
Thanks!
@@ -56,6 +57,7 @@ | |||
* call to {@link #initCause}. | |||
*/ | |||
@SideEffectFree | |||
@Unique |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write as public @Unique Exception
to put the return type annotation before the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the changes in the latest commit
@@ -248,8 +249,10 @@ | |||
* an empty character sequence. Note that use of this constructor is | |||
* unnecessary since Strings are immutable. | |||
*/ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove superfluous empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the empty line in my new commit
@SideEffectFree | ||
@StaticallyExecutable | ||
@Unique |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put type use annotations before the types they modify. Here, after the existing @StringVal
to sort them alphabetically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the annotation location (in alphabetical order) in my latest commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typetools/checker-framework#3336 should also remove the stub file, right?
@@ -56,7 +57,7 @@ | |||
* call to {@link #initCause}. | |||
*/ | |||
@SideEffectFree | |||
public Exception() { | |||
public @Unique Exception() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other constructors for this class. Shouldn't they also return a @Unique
object?
Can you just look through the whole class bodies for the two methods and see whether other annotations are missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have annotated the remaining constructors as @unique in my latest commit.
@@ -62,7 +63,7 @@ | |||
* Constructs a new object. | |||
*/ | |||
@HotSpotIntrinsicCandidate | |||
public Object() {} | |||
public @Unique Object() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already in master? Please pull in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for the difference is that I had pushed these changes before my other PR was merged. I have pulled in master in my latest commit.
@@ -250,7 +251,7 @@ | |||
*/ | |||
@SideEffectFree | |||
@StaticallyExecutable | |||
public @StringVal("") String() { | |||
public @StringVal("") @Unique String() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for this class and StringBuffer below, annotate the other constructors and see whether any other methods should be annotated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have annotated the remaining constructors as @unique in my latest commit. I also annotated the append function for the StringBuffer data type as they were annotated in the stub file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! One questions, but otherwise looks good and is an improvement to what used to be in the stub file.
@@ -153,7 +154,7 @@ | |||
* @jls 15.18.1 String Concatenation Operator + | |||
*/ | |||
|
|||
@AnnotatedFor({"formatter", "index", "interning", "lock", "nullness", "regex", "signature", "signedness", "value"}) | |||
@AnnotatedFor({"aliasing", "formatter", "index", "interning", "lock", "nullness", "regex", "signature", "signedness"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you go through all methods and make sure uniqueness is correctly annotated? Same for StringBuffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through all the methods, however, I don't think any of them should return @unique results. Hence, I annotated the constructors of the data types for the most part. In StringBuffer, I also annotated the append function as they were annotated in the now deleted stubfile.
This reverts commit 4891771
This PR adds the @unique annotation to the classes written in the stubfile.astub file in the tests/aliasing folder. By adding these annotations to the annotated jdk, the stub file becomes redundant.
This PR needs to be merged with the 3313 explicit annotation fix PR in typetools/checker-framework.
(typetools/checker-framework#3336)