Annotating classes of util package #1

Kakarot-SSJ4 · 2019-07-12T15:19:31Z

Annotated the classes MathArrays.java, MedianOf3PivotingStrategy.java and MultidimensionalCounter.java

kelloggm

Looks good overall. I had some relatively minor comments.

kelloggm · 2019-07-12T20:22:01Z

pom.xml

+        </plugin>
+        <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-compiler-plugin</artifactId>


This looks like a second copy of the maven-compiler-plugin, which is the same problem we had with commons-lang earlier today.

kelloggm · 2019-07-12T20:23:07Z

src/main/java/org/apache/commons/math4/util/CentralPivotingStrategy.java

@@ -38,7 +43,7 @@
     * @throws MathIllegalArgumentException when indices exceeds range
     */
    @Override
-    public int pivotIndex(final double[] work, final int begin, final int end)
+    public int pivotIndex(final double[] work, final @LessThan("#3") @IndexFor("#1") int begin, final @Positive @LTEqLengthOf("#1") int end)


Shouldn't the return type of this method be @IndexFor("#1")?

kelloggm · 2019-07-12T21:27:48Z

src/main/java/org/apache/commons/math4/util/MathArrays.java

@@ -317,7 +328,7 @@ public static int distanceInf(int[] p1, int[] p2)
     * @param strict Whether the order should be strict.
     * @return {@code true} if sorted, {@code false} otherwise.
     */
-    public static  <T extends Comparable<? super T>> boolean isMonotonic(T[] val,
+    public static  <T extends Comparable<? super T>> boolean isMonotonic(@PolyUpperBound @PolyLowerBound @PolySameLen T @MinLen(1) [] val,


Should change these to @PolyAll

kelloggm · 2019-07-12T21:31:37Z

src/main/java/org/apache/commons/math4/util/MathArrays.java

@@ -823,10 +838,11 @@ public int compare(PairDoubleInteger o1,
     * @param to Final index of the range to be copied, exclusive. (This index may lie outside the array.)
     * @return the copied array.
     */
-    public static double[] copyOfRange(double[] source, int from, int to) {
+    @SuppressWarnings("index:argument.type.incompatible") // #1: As FastMath.min(len, source.length - from)) is the minimum of both the arguments, the expression <= source.length - fron and <= output


This part of the comment: "the expression <= source.length - fron and <= output" is very unclear - what does "the expression" refer to? There's also a typo: "fron" should be "from".

kelloggm · 2019-07-12T21:47:05Z

src/main/java/org/apache/commons/math4/util/MultidimensionalCounter.java

-    public MultidimensionalCounter(int ... size) throws NotStrictlyPositiveException {
-        dimension = size.length;
-        this.size = MathArrays.copyOf(size);
+    @SuppressWarnings("index:assignment.type.incompatible") // #1: By #0.1, this.size.length = size.length


Why not annotate/stub MathArrays.copyOf to return @SameLen("#1")?

kelloggm · 2019-07-12T21:47:54Z

src/main/java/org/apache/commons/math4/util/MultidimensionalCounter.java

@@ -223,6 +231,7 @@ public int getDimension() {
     * @throws OutOfRangeException if {@code index} is not between
     * {@code 0} and the value returned by {@link #getSize()} (excluded).
     */
+    @SuppressWarnings("index:array.access.unsafe.high") // #1: i < last < dimension which is the length of indices and uniCounterOffset
    public int[] getCounts(int index) throws OutOfRangeException {


Surely index should have an annotation here. If nothing else, it should definitely be @NonNegative to avoid the exception below.

kelloggm

This is looking better. Nice work.

kelloggm · 2019-07-19T22:10:48Z

src/main/java/org/apache/commons/math4/util/CompositeFormat.java

@@ -174,7 +177,7 @@ public static boolean parseFixedstring(final String source,
        final int endIndex = startIndex + expected.length();
        if ((startIndex >= source.length()) ||
            (endIndex > source.length()) ||
-            (source.substring(startIndex, endIndex).compareTo(expected) != 0)) {
+            (source.substring(startIndex, endIndex).compareTo(expected) != 0)) { // #1


I would expect this call to substring to not issue an error, because the expected type for both arguments is IndexOrHigh - which should both be established by the conditionals, as your suppress warning comment notes. Can you write a test case like this code and submit a bug? Thanks!

The explanation @SuppressWarnings was wrong, it actually shows array.access.unsafe.low error as it cannot recognise pos.getIndex() as @NonNegative.
The PR with ParsePosition annotation is open, so I have provided a link in the comment.

kelloggm · 2019-07-19T22:11:44Z

src/main/java/org/apache/commons/math4/util/KthSelector.java

+    @SuppressWarnings({"index:array.access.unsafe.low", "index:argument.type.incompatible"}) /*
+    #1: node is always @NonNegative as it is changed only in #0.1 where it is minimum of 2*node + 1(or 2) and either pivotsHeap.length or end
+        pivotsHeap.length is @NonNegative as it is a length, end is also @NonNegative as it is initialized 0 and changed only in #0.2 where end = pivot >= k that is @NonNegative
+    #3: begin and end are @NonNegative as t is assign


What does "as t is assign" mean?

Kakarot-SSJ4 · 2019-07-25T17:25:13Z

@kelloggm @panacekcz

kelloggm

Only minor issues here, but the Travis build is failing, which you should look into.

kelloggm · 2019-07-25T17:31:23Z

src/main/java/org/apache/commons/math4/util/KthSelector.java

@@ -83,11 +83,11 @@ public PivotingStrategyInterface getPivotingStrategy() {
    @SuppressWarnings({"index:array.access.unsafe.low", "index:argument.type.incompatible"}) /*
    #1: node is always @NonNegative as it is changed only in #0.1 where it is minimum of 2*node + 1(or 2) and either pivotsHeap.length or end
        pivotsHeap.length is @NonNegative as it is a length, end is also @NonNegative as it is initialized 0 and changed only in #0.2 where end = pivot >= k that is @NonNegative
-    #3: begin and end are @NonNegative as t is assign
+    #3: begin and end are @NonNegative as it is initialised with @NonNegative values in #0. and changed to only @NonNegative values in #0.2 and #0.4


What does "#0." mean here? Are you referring to "#0.3"?

Also, a minor nit on English grammar: "it" should only be used for a singular antecedent. In this case, the antecedent is "begin and end", which is plural; therefore, the appropriate pronoun is "they". The sentence should read: "begin and end are @NonNegative as they are initialised..."

Kakarot-SSJ4 · 2019-08-01T19:12:59Z

@kelloggm It is ready for review again.

panacekcz · 2019-08-16T14:54:19Z

pom.xml

@@ -516,6 +530,47 @@

  <build>
      <plugins>
+          <plugin>
+    <!-- This plugin will set properties values using dependency information -->


Fix indentation in this file.

panacekcz · 2019-08-16T15:00:56Z

src/main/java/org/apache/commons/math4/util/MathArrays.java

-    @SuppressWarnings("unchecked")
-    public static <T> T[][] buildArray(final Field<T> field, final int rows, final int columns) {
+    @SuppressWarnings({"unchecked","index:array.access.unsafe.high.range"}) // i < rows which is the number of rows in array
+    public static <T> T[][] buildArray(final Field<T> field, final @NonNegative int rows, final @NonNegative int columns) {


The documentation says that columns may be negative.

panacekcz · 2019-08-16T15:02:08Z

src/main/java/org/apache/commons/math4/util/MathArrays.java

+    @SuppressWarnings({"index:array.access.unsafe.high","index:array.access.unsafe.low"}) /*
+    #1: j >= 0 is checked and j < xLen as j is max n + 1 - xLen where n is max xLen + hLen - 2
+        k < hLen is checked
+    */


Looks like the parameters should be @MinLen(1)

panacekcz · 2019-08-16T15:18:09Z

pom.xml

+      <compilerArgs>
+        <!-- location of the annotated JDK, which comes from a Maven dependency -->
+        <arg>-Xbootclasspath/p:${annotatedJdk}</arg>
+        <arg>-AonlyDefs=^org\.apache\.commons\.math4\.util\.</arg>


Can you also add -ArequirePrefixInWarningSuppressions and -AwarnUnneededSuppressions as in commons-lang?

panacekcz · 2019-08-16T15:31:58Z

src/main/java/org/apache/commons/math4/util/KthSelector.java

-        int begin = 0;
-        int end = work.length;
+    @SuppressWarnings({"index:array.access.unsafe.low", "index:argument.type.incompatible"}) /*
+    #1: node is always @NonNegative as it is changed only in #0.1 where it is minimum of 2*node + 1(or 2) and either pivotsHeap.length or end


Then node should be annotated @NonNegative. Possibly begin and end as well.

Kakarot-SSJ4 · 2019-08-16T20:59:55Z

@panacekcz made the changes you suggested

Annotated OpenIntToFieldHashMap.java and FastMathCalc.java

Annotated OpenIntToDoubleHashMap.java and FastMath.java

Kakarot-SSJ4 · 2019-08-21T20:48:17Z

Pull requests merged:
Kakarot-SSJ4#3
Kakarot-SSJ4#1

Kakarot-SSJ4 · 2019-08-21T21:49:31Z

@kelloggm @panacekcz This is ready to be merged, if you approve all the changes.

panacekcz

The annotations look good. See comments about missing reference. Other than that, I think this is ready to be merged.

panacekcz · 2019-08-27T11:11:40Z

src/main/java/org/apache/commons/math4/util/OpenIntToFieldHashMap.java

@@ -566,6 +590,9 @@ public T value()
         * @exception ConcurrentModificationException if the map is modified during iteration
         * @exception NoSuchElementException if there is no element left in the map
         */
+        @SuppressWarnings({"index:assignment.type.incompatible", "index:array.access.unsafe.low", "index:array.access.unsafe.high", "index:unary.increment.type.incompatible"}) /*
+        #1: These statement is never executed with next = states.length because when next = states.length - 1, the try catch block makes next = -2


It's not clear what statement this refers to - I presume it is the ++next expression.

panacekcz · 2019-08-27T11:14:44Z

src/main/java/org/apache/commons/math4/util/ResizableDoubleArray.java

@@ -267,6 +275,7 @@ public ResizableDoubleArray(int initialCapacity, double expansionFactor, double
     * @throws MathIllegalArgumentException if the parameters are not valid.
     * @throws NullArgumentException if expansionMode is null
     */
+    @SuppressWarnings("index:assignment.type.incompatible") // internalArray is @MinLen(1) and startIndex is @IndexFor("internalArray"), hence 0 < 1 - 0 + 1


Does this refer to #1?

Kakarot-SSJ4 · 2019-08-29T20:46:11Z

@panacekcz updated with the review

panacekcz

@kelloggm, do you want to review again? Otherwise I think it can be merged.

Kakarot-SSJ4 added 7 commits July 12, 2019 13:17

annotating

adde8f7

MultidimesnionalCounter

a1e270d

pom.xml

f0c7f44

unnecessary changes

91e7996

pom

5187fb2

recitfying

9170b66

removed unnecessary changes

0129347

kelloggm requested changes Jul 12, 2019

View reviewed changes

Kakarot-SSJ4 added 3 commits July 15, 2019 20:35

annotating

2940ec6

made suggested changes

a87a7fe

annotating FastMath.java

428f5fd

kelloggm approved these changes Jul 19, 2019

View reviewed changes

Kakarot-SSJ4 and others added 15 commits July 21, 2019 14:53

OpenIntToDoubleHashMap.java

9ec2c44

OpenIntToFieldHashMap.java

d9f54c8

annotating

1bf39c6

fixing

a57e7d0

fixing

6cee18b

made suggested changes

dfe72f2

made suggested changes

a74ac32

made suggested changes

c608b71

made suggested changes

00e9867

annotated FastMathCalc.java

112248e

checker version 2.9.0

f3cfb7e

made suggested changes

855cf84

made suggested changes

e96b1a7

made suggested changes

ca5ae72

Merge branch 'annotated-2' into annotated-3

abee324

kelloggm requested changes Jul 25, 2019

View reviewed changes

made suggested changes

5bbf6e7

Kakarot-SSJ4 and others added 4 commits July 25, 2019 23:50

Merge branch 'annotated' into annotated-2

915e208

Merge branch 'annotated-2' into annotated-3

39f4314

message

dbb06b9

openjdk

4e439f5

Kakarot-SSJ4 added 3 commits August 16, 2019 20:33

IntRange

53d47ef

@nonnegative

473c6de

error handled

a59a8c5

panacekcz suggested changes Aug 16, 2019

View reviewed changes

made suggested changes

07fe742

Kakarot-SSJ4 added 3 commits August 22, 2019 01:58

Merge pull request #3 from Kakarot-SSJ4/annotated-3

566f7af

Annotated OpenIntToFieldHashMap.java and FastMathCalc.java

Merge branch 'annotated' into annotated-2

1102c66

Merge pull request #1 from Kakarot-SSJ4/annotated-2

a91c465

Annotated OpenIntToDoubleHashMap.java and FastMath.java

Kakarot-SSJ4 added 3 commits August 22, 2019 02:21

recctifying errors

aed29ed

resolved all the errors

dd1859e

skip test files

623e052

panacekcz approved these changes Aug 27, 2019

View reviewed changes

made suggested changes

2cf074f

panacekcz approved these changes Aug 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotating classes of util package #1

Annotating classes of util package #1

Kakarot-SSJ4 commented Jul 12, 2019

kelloggm left a comment

kelloggm Jul 12, 2019

kelloggm Jul 12, 2019

kelloggm Jul 12, 2019

kelloggm Jul 12, 2019

kelloggm Jul 12, 2019

kelloggm Jul 12, 2019

kelloggm left a comment

kelloggm Jul 19, 2019

Kakarot-SSJ4 Jul 23, 2019

kelloggm Jul 19, 2019

Kakarot-SSJ4 commented Jul 25, 2019

kelloggm left a comment

kelloggm Jul 25, 2019

kelloggm Jul 25, 2019

Kakarot-SSJ4 commented Aug 1, 2019

panacekcz Aug 16, 2019

panacekcz Aug 16, 2019

panacekcz Aug 16, 2019

panacekcz Aug 16, 2019

panacekcz Aug 16, 2019

Kakarot-SSJ4 commented Aug 16, 2019

Kakarot-SSJ4 commented Aug 21, 2019

Kakarot-SSJ4 commented Aug 21, 2019

panacekcz left a comment

panacekcz Aug 27, 2019

panacekcz Aug 27, 2019

Kakarot-SSJ4 commented Aug 29, 2019

panacekcz left a comment

Annotating classes of util package #1

Are you sure you want to change the base?

Annotating classes of util package #1

Conversation

Kakarot-SSJ4 commented Jul 12, 2019

kelloggm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelloggm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kakarot-SSJ4 commented Jul 25, 2019

kelloggm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kakarot-SSJ4 commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kakarot-SSJ4 commented Aug 16, 2019

Kakarot-SSJ4 commented Aug 21, 2019

Kakarot-SSJ4 commented Aug 21, 2019

panacekcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kakarot-SSJ4 commented Aug 29, 2019

panacekcz left a comment

Choose a reason for hiding this comment