This repository has been archived by the owner on May 7, 2020. It is now read-only.
added some null annotations to core APIs #3933
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,16 @@ | |
| import java.util.Collection; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import org.eclipse.jdt.annotation.NonNull; | ||
| import org.eclipse.jdt.annotation.Nullable; | ||
|
|
||
| /** | ||
| * The {@link Registry} interface represents a registry for elements of the type | ||
| * E. The concrete sub interfaces are registered as OSGi services. | ||
| * | ||
| * @author Dennis Nobel - Initial contribution | ||
| * @author Victor Toni - provide elements as {@link Stream} | ||
| * @author Kai Kreuzer - added null annotations | ||
| * | ||
| * @param <E> type of the elements in the registry | ||
| */ | ||
|
|
@@ -26,14 +30,15 @@ public interface Registry<E extends Identifiable<K>, K> { | |
| * | ||
| * @param listener registry change listener | ||
| */ | ||
| void addRegistryChangeListener(@NonNull RegistryChangeListener<E> listener); | ||
|
|
||
| /** | ||
| * Returns a collection of all elements in the registry. | ||
| * | ||
| * @return collection of all elements in the registry | ||
| */ | ||
| @NonNull | ||
| Collection<@NonNull E> getAll(); | ||
|
|
||
| /** | ||
| * Returns a stream of all elements in the registry. | ||
|
|
@@ -49,15 +54,15 @@ public interface Registry<E extends Identifiable<K>, K> { | |
| * key of the element | ||
| * @return element or null if no element was found | ||
| */ | ||
| public @Nullable E get(K key); | ||
|
There was a problem hiding this comment. I don't see what information gives this annotation here. There was a problem hiding this comment. It says that the return value might be null (and by adding it, we state that this is deliberate and not that we merely didn't bother adding a There was a problem hiding this comment. My concerns was that exactly this was said in the javadoc (but in case if it is not read ok) and it is obvious for me that it will return a null for any key without value (as a Map). There was a problem hiding this comment.
Well, there are also other examples, so making it explicit is a good thing. There was a problem hiding this comment. You can change the return value from It is the same logic used for more specialized returned classes used by the JVM. There was a problem hiding this comment. @maggu2810 these are two different methods and the getItem() does not overwrite the get(), My proposal was: Registry.get() to be not annotated, because it is obvious that it returns null for missing elements (as any Map), and to annotated @nonnull ItemRegistry.getItem() because it never return a null. |
||
|
|
||
| /** | ||
| * Removes a {@link RegistryChangeListener} from the registry. | ||
| * | ||
| * @param listener | ||
| * registry change listener | ||
| */ | ||
| void removeRegistryChangeListener(@NonNull RegistryChangeListener<E> listener); | ||
|
|
||
| /** | ||
| * Adds the given element to the according {@link ManagedProvider}. | ||
|
|
@@ -68,7 +73,7 @@ public interface Registry<E extends Identifiable<K>, K> { | |
| * @throws IllegalStateException | ||
| * if no ManagedProvider is available | ||
| */ | ||
| public @NonNull E add(@NonNull E element); | ||
|
|
||
| /** | ||
| * Updates the given element at the according {@link ManagedProvider}. | ||
|
|
@@ -80,7 +85,7 @@ public interface Registry<E extends Identifiable<K>, K> { | |
| * @throws IllegalStateException | ||
| * if no ManagedProvider is available | ||
| */ | ||
| public @Nullable E update(@NonNull E element); | ||
|
|
||
| /** | ||
| * Removes the given element from the according {@link ManagedProvider}. | ||
|
|
@@ -92,5 +97,5 @@ public interface Registry<E extends Identifiable<K>, K> { | |
| * @throws IllegalStateException | ||
| * if no ManagedProvider is available | ||
| */ | ||
| public @Nullable E remove(@NonNull K key); | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it possible the AbstractRegistry to return a null value for already added element? If so it is a bug in the AbstractRegistry implementation for me.
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.
No, it is a bug in your train of thoughts - which proves that the annotations are helpful :-)
We are requesting a rule of a certain UID here. You cannot assume that it exists here. Although you have added it previously, somebody else might have deleted it again or the
initRuleIdmethod might have decided to store it under a different UID - how could you tell?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 initRuleId won't change id because it is my method, but you are right that someone can meanwhile remove the added element. Ok this approach is better then synchronization for me.
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 is no such thing as "my method" in an open source project. Make sure to implement robust code that does not do too many implicit assumptions that only exist in your mind.
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 mean that this is an implementation detail which is not exposed by the API. We have to find a limit where to stop with annotations. Do we have to set annotation on the private methods too?
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.
If they are helpful as they remove a warning at another place as in this case, what speaks against it?
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 don't have a worning in places where I use this method. Btw these warnings are related with missing null checks. Isn't be better to have handle these situations (as we saw that @nonnull does not defends us) and provider a suitable exception instead of just a NullPointerException at runtime.
Anyway I can live with this change.
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.
It actually defends us especially on internal methods, because we have checks in place in our IDE and the Maven build. So we can be sure that all ESH code itself respects the annotations - imho definitely helpful for any kind of future refactorings.