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

#48 ZIP file to PR, java runtime library #56

Closed
wants to merge 1 commit into from

Conversation

Bananeweizen
Copy link
Contributor

@Bananeweizen Bananeweizen commented Nov 11, 2017

This is the merged ZIP file contribution for the Java runtime.

  • all signature changes of existing methods in java/java have been reviewed by me (and only 4 of the
    suggested changes were rejected by me).
  • changes in java/javax have not been reviewed.
  • new method signatures have not been reviewed (but will be crosschecked over the next days by using them in my own projects).

This is the merged ZIP file contribution for the Java runtime. All
changes in java/java have been reviewed by me (and only 4 of the
suggested changes were rejected by me). Changes in java/javax have not
been reviewed.
@@ -13,7 +61,7 @@ contains
(L1java/lang/CharSequence;)Z
contentEquals
(Ljava/lang/CharSequence;)Z
(L0java/lang/CharSequence;)Z
(L1java/lang/CharSequence;)Z
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one really was wrong before, the argument is dereferenced.

intern
()Ljava/lang/String;
()L1java/lang/String;
join
(Ljava/lang/CharSequence;Ljava/lang/Iterable<+Ljava/lang/CharSequence;>;)Ljava/lang/String;
(L1java/lang/CharSequence;L1java/lang/Iterable<+1L1java/lang/CharSequence;>;)L1java/lang/String;
(L1java/lang/CharSequence;L1java/lang/Iterable<+L0java/lang/CharSequence;>;)L1java/lang/String;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this and the one below were wrong before. The joined collections must be non-null, however the elements in there can be null, since StringBuilder deals with appending null.

@@ -22,16 +25,19 @@ from
(L1java/time/temporal/TemporalAmount;)L1java/time/Duration;
get
(Ljava/time/temporal/TemporalUnit;)J
(L0java/time/temporal/TemporalUnit;)J
(L1java/time/temporal/TemporalUnit;)J
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of "interpretation dependent". Null is accepted, but will throw UnsupportedTemporalTypeException, therefore it makes sense to assume NonNull.

@Bananeweizen
Copy link
Contributor Author

Note for reviewers: I've marked the 4 places with actual signature changes in the review. Everything else is just new or more annotations. So if you review those 4 places, you can afterwards accept and merge rather easily.

@vorburger vorburger mentioned this pull request Nov 13, 2017
Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

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

LGTM - it seems like @Bananeweizen you've spent some time looking these over.. I'm fine to merge this as is, but want to give others a chance to review.

Let's say that if there are no other feedback (e.g. from @maggu2810 @sylvainlaurent @kwin@ctron ) within 1 week, then I'll merge this one without further comment.

@maggu2810
Copy link
Contributor

I would give it a go. If there is some stuff wrong, it could be fixed later.

(L0java/lang/Object;)Z
getInteger
(Ljava/lang/String;)Ljava/lang/Integer;
(L0java/lang/String;)L1java/lang/Integer;
Copy link
Member

Choose a reason for hiding this comment

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

incorrect, getInteger(String) may return null

(L1java/util/Collection<+TE;>;)Z
contains
(Ljava/lang/Object;)Z
(Ljava/lang/Object;)Z
Copy link
Member

Choose a reason for hiding this comment

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

accepts null param

()L1java/util/stream/Stream<T+E;>;
remove
(Ljava/lang/Object;)Z
(Ljava/lang/Object;)Z
Copy link
Member

Choose a reason for hiding this comment

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

accepts null param


append
(Ljava/lang/CharSequence;II)Ljava/io/StringWriter;
(L1java/lang/CharSequence;II)L1java/io/StringWriter;
Copy link
Member

Choose a reason for hiding this comment

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

null CharSequence is actually accepted

@@ -1,13 +1,169 @@
class java/lang/Class
classValueMap
Ljava/lang/ClassValue$ClassValueMap;
Ljava/lang/ClassValue$ClassValueMap;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think user code would ever reference this field

Copy link
Contributor Author

@Bananeweizen Bananeweizen Nov 22, 2017

Choose a reason for hiding this comment

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

This one is from the original contribution, not from me. However, I tend to add annotations for private fields also during the analysis, if there is some proof. That makes it easier to later annotate their getters and related methods, even if those don't immediately proof null/nonnull on their own. What do others think about that?

Copy link
Member

Choose a reason for hiding this comment

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

my view on this point would be that if it doesn't cause any harm, then why not


out
Ljava/io/PrintStream;
L0java/io/PrintStream;
Copy link
Member

Choose a reason for hiding this comment

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

this is technically correct, but it never occurs that it's null. And we would have eclipse warnings/errors for a mere System.out.println(...)
same thing for in and err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have had similar doubts. In my personal project I have meanwhile also removed this annotation for the same reason of it not being pragmatic.

Choose a reason for hiding this comment

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

It is possible for System.out to be null. Some piece of code can set it to null. I am not sure why they would do that in the general case. However, I have seen it done in 1 case.

Also, by marking it nullable, then the programmer has to go through more hoops to use it. This encourages using a logger instead.

@@ -13,4 +28,13 @@ ofNullable
<T:Ljava/lang/Object;>(T0T;)L1java/util/Optional<TT;>;
orElse
(TT;)TT;
(T0T;)TT;
(T0T;)T0T;
Copy link
Member

Choose a reason for hiding this comment

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

debatable... I wish there was an equivalent to @contract or @polynull in jdt


setProperty
(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/Object;
(L0java/lang/String;L0java/lang/String;)L0java/lang/Object;
Copy link
Member

Choose a reason for hiding this comment

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

incorrect: neither key nor value can be null


poll
(JLjava/util/concurrent/TimeUnit;)TE;
(JL1java/util/concurrent/TimeUnit;)T1E;
Copy link
Member

Choose a reason for hiding this comment

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

according to java.util.concurrent.BlockingQueue.poll(long, TimeUnit) null is returned if times out without thread interruption

@vorburger
Copy link
Member

@Bananeweizen are you planning to update this one some time, based on @sylvainlaurent feedback?

@vorburger
Copy link
Member

If anyone is reading this and wants to rebase this PR and resolve merge conflicts, we could merge it.

@vorburger
Copy link
Member

Maybe @simoneparca is interested? Given his #89.

@J-N-K
Copy link
Member

J-N-K commented Mar 4, 2021

I would really appreciate seeing the missing annotations, but since this did not have activity for a year now and can't be merged due to conflicts, I'm closing here. I promise that I'll review ASAP if someone comes up with a new PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants