Skip to content
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

Merged
merged 11 commits into from
Jul 21, 2020

Conversation

aditya3434
Copy link

@aditya3434 aditya3434 commented Jun 20, 2020

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)

@wmdietl
Copy link
Member

wmdietl commented Jun 24, 2020

Please add a link to the corresponding checker-framework PR that removes that stub file.

Copy link
Member

@wmdietl wmdietl left a 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
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove superfluous empty line.

Copy link
Author

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
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

@wmdietl wmdietl left a 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() {
Copy link
Member

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?

Copy link
Author

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() {}
Copy link
Member

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.

Copy link
Author

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() {
Copy link
Member

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.

Copy link
Author

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.

@aditya3434 aditya3434 assigned wmdietl and unassigned aditya3434 Jun 28, 2020
Copy link
Member

@wmdietl wmdietl left a 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"})
Copy link
Member

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.

Copy link
Author

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.

@wmdietl wmdietl assigned aditya3434 and unassigned wmdietl Jul 20, 2020
@aditya3434 aditya3434 assigned wmdietl and unassigned aditya3434 Jul 21, 2020
@wmdietl wmdietl changed the title 3313 explicit annotation fix Add aliasing annotations Jul 21, 2020
@wmdietl wmdietl merged commit 4891771 into typetools:master Jul 21, 2020
@aditya3434 aditya3434 deleted the 3313-explicit-annotation-fix branch July 27, 2020 18:29
xingweitian added a commit to opprop/jdk that referenced this pull request Sep 29, 2020
Ao-senXiong pushed a commit to Ao-senXiong/jdkopprop that referenced this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants