RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Matthias Baesken
JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

-------------

Commit messages:
 - JDK-8261422

Changes: https://git.openjdk.java.net/jdk/pull/2483/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2483&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261422
  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2483.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2483/head:pull/2483

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Matthias Baesken
On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

The method outOfBoundsMessage has a few problematic calls to String.format.
Those calls use "%d" however the arguments are of type Number, so we should better use %s .

Example :

https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM87A8sPJZZzON5wY&resolved=false&severities=BLOCKER&types=BUG

return String.format("Index %d out of bounds for length %d",
                     args.get(0), args.get(1));

Sonar error :
An 'int' is expected rather than a .

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Alan Bateman-2
In reply to this post by Matthias Baesken
On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

src/java.base/share/classes/jdk/internal/util/Preconditions.java line 212:

> 210:                                      args.get(0), args.get(1), args.get(2));
> 211:             case "checkFromIndexSize":
> 212:                 return String.format("Range [%s, %<s + %s) out of bounds for length %s",

I assume we have a code coverage here and that we need tests to exercise these code paths.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Matthias Baesken
On Tue, 9 Feb 2021 15:38:29 GMT, Alan Bateman <[hidden email]> wrote:

>> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
>
> src/java.base/share/classes/jdk/internal/util/Preconditions.java line 212:
>
>> 210:                                      args.get(0), args.get(1), args.get(2));
>> 211:             case "checkFromIndexSize":
>> 212:                 return String.format("Range [%s, %<s + %s) out of bounds for length %s",
>
> I assume we have a code coverage here and that we need tests to exercise these code paths.

I haven't written the class Preconditions.java, this question should probably go to the authors .
I found the String.format syntax a bit  unusual,  should this be rewritten ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

stefan-zobel
On Tue, 9 Feb 2021 15:47:39 GMT, Matthias Baesken <[hidden email]> wrote:

>> src/java.base/share/classes/jdk/internal/util/Preconditions.java line 212:
>>
>>> 210:                                      args.get(0), args.get(1), args.get(2));
>>> 211:             case "checkFromIndexSize":
>>> 212:                 return String.format("Range [%s, %<s + %s) out of bounds for length %s",
>>
>> I assume we have a code coverage here and that we need tests to exercise these code paths.
>
> I haven't written the class Preconditions.java, this question should probably go to the authors .
> I found the String.format syntax a bit  unusual,  should this be rewritten ?

AFAIK, Preconditions was introduced in https://bugs.openjdk.java.net/browse/JDK-8155794

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Christoph Langer
In reply to this post by Matthias Baesken
On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

As we're potentially formatting any "Number" type here, theoretically floats could be passed which would result in an Exception. In fact, only decimal types are passed by the callers so this should not happen. Unfortunately there's no super type specifying decimal numbers.

And as we're not doing some fancy formatting but just %d, I think replacing this with %s should be ok.

-------------

Marked as reviewed by clanger (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Brian Burkhalter-3
In reply to this post by stefan-zobel
On Tue, 9 Feb 2021 16:04:02 GMT, stefan-zobel <[hidden email]> wrote:

>> I haven't written the class Preconditions.java, this question should probably go to the authors .
>> I found the String.format syntax a bit  unusual,  should this be rewritten ?
>
> AFAIK, Preconditions was introduced in https://bugs.openjdk.java.net/browse/JDK-8155794

It looks like java/util/Objects/CheckIndex should adequately cover this change.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Integrated: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Matthias Baesken
In reply to this post by Matthias Baesken
On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

This pull request has now been integrated.

Changeset: 219b115e
Author:    Matthias Baesken <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/219b115e
Stats:     4 lines in 1 file changed: 0 ins; 0 del; 4 mod

8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Reviewed-by: clanger

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Alan Bateman-2
In reply to this post by Christoph Langer
On Fri, 12 Feb 2021 11:02:36 GMT, Christoph Langer <[hidden email]> wrote:

>> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
>
> As we're potentially formatting any "Number" type here, theoretically floats could be passed which would result in an Exception. In fact, only decimal types are passed by the callers so this should not happen. Unfortunately there's no super type specifying decimal numbers.
>
> And as we're not doing some fancy formatting but just %d, I think replacing this with %s should be ok.

I see this change been integrated but there is further work required on the test coverage. Are you planing to do code coverage and create a follow-on issue to add more tests?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483