Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v9]
On Thu, 12 May 2022 14:56:47 GMT, Daniel Fuchs wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Typo Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v9]
On Thu, 12 May 2022 14:15:43 GMT, Daniel Fuchs wrote: >>> OK - I will change codeLengthOf as suggested. It was not immediately >>> obvious to me that the values would fit in the first 31 bits. >> >> In fact, it would even fit into the first 30 bits. There's a top-level >> comment that explains the layout of `code` elements. Maybe you can improve >> it somehow or carry over that specific part about the length into >> `codeLengthOf`. Alternatively, you can always slap `assert` in >> `codeLengthOf`. > > Did I understand correctly that the lower 32 bits is expected to contain a > length coded on 5 bits - which is expected to be a value in (5..30)? Yes, you did understand that correctly. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v9]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Typo - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/1f0902ed..258a5897 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v8]
On Thu, 12 May 2022 14:25:44 GMT, Daniel Fuchs wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Move assert Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v8]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Move assert - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/25c24d67..1f0902ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=06-07 Stats: 4 lines in 1 file changed: 2 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v7]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Pavel's feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/deb22998..25c24d67 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=05-06 Stats: 8 lines in 2 files changed: 4 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v7]
On Thu, 12 May 2022 11:41:04 GMT, Pavel Rappo wrote: >> OK - I will change codeLengthOf as suggested. It was not immediately >> obvious to me that the values would fit in the first 31 bits. > >> OK - I will change codeLengthOf as suggested. It was not immediately obvious >> to me that the values would fit in the first 31 bits. > > In fact, it would even fit into the first 30 bits. There's a top-level > comment that explains the layout of `code` elements. Maybe you can improve it > somehow or carry over that specific part about the length into > `codeLengthOf`. Alternatively, you can always slap `assert` in `codeLengthOf`. Did I understand correctly that the lower 32 bits is expected to contain a length coded on 5 bits - which is expected to be a value in (5..30)? - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v6]
On Thu, 12 May 2022 10:51:04 GMT, Daniel Fuchs wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > lengthOfCode() returns int; Removed excessive comments Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v6]
On Thu, 12 May 2022 10:05:14 GMT, Daniel Fuchs wrote: > OK - I will change codeLengthOf as suggested. It was not immediately obvious > to me that the values would fit in the first 31 bits. In fact, it would even fit into the first 30 bits. There's a top-level comment that explains the layout of `code` elements. Maybe you can improve it somehow or carry over that specific part about the length into `codeLengthOf`. Alternatively, you can always slap `assert` in `codeLengthOf`. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 10:08:23 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
>> line 291:
>>
>>> 289:
>>> 290: HeaderWriter noMask() {
>>> 291: // The negation "~" sets the high order bits
>>
>> Rubber-stamping this in front of every of the four closely sitting casts
>> seems excessive:
>>
>> // The negation "~" sets the high order bits
>> // so the value is more than 16 bits and the
>> // compiler will emit a warning if not cast
>
> I don't disagree. I had the same feeling. But I don't necessarily read a file
> from top to bottom - and someone just glancing at one of these methods might
> wonder why one line has the cast and the other hasn't.
Readers should always consider as much of the context as practical; but I like
your solution.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v6]
On Thu, 12 May 2022 10:51:04 GMT, Daniel Fuchs wrote:
>> In relation to
>> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find
>> here a patch that addresses possibly lossy conversions in java.net.http
>
> Daniel Fuchs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> lengthOfCode() returns int; Removed excessive comments
src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java
line 736:
> 734: }
> 735: int len = codeLengthOf(c);
> 736: assert len >= 0;
This `assert` could be better if placed into `codeLengthOf`, which is called
from one more place.
src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line
220:
> 218: // Explicit cast required:
> 219: // The negation "~" sets the high order bits
> 220: // so the value is more than 16 bits and the
Consider using: the value *becomes longer* than 16 bits
src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line
287:
> 285: HeaderWriter noMask() {
> 286: // The negation "~" sets the high order bits
> 287: // so the value is more than 16 bits and the
Again, consider: *becomes longer*
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v6]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: lengthOfCode() returns int; Removed excessive comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/2334b747..deb22998 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=04-05 Stats: 16 lines in 2 files changed: 2 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 09:15:19 GMT, Pavel Rappo wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert adding char constants
>
> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
> line 291:
>
>> 289:
>> 290: HeaderWriter noMask() {
>> 291: // The negation "~" sets the high order bits
>
> Rubber-stamping this in front of every of the four closely sitting casts
> seems excessive:
>
> // The negation "~" sets the high order bits
> // so the value is more than 16 bits and the
> // compiler will emit a warning if not cast
I don't disagree. I had the same feeling. But I don't necessarily read a file
from top to bottom - and someone just glancing at one of these methods might
wonder why one line has the cast and the other hasn't.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 09:00:37 GMT, Pavel Rappo wrote:
>> This is what I mean:
>>
>> jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1
>> codeLengthOf ==> 2147483648
>>
>> jshell> int bufferLen = 0
>> bufferLen ==> 0
>>
>> jshell> bufferLen + codeLengthOf <= 64
>> $3 ==> false
>>
>> jshell> bufferLen + (int)codeLengthOf <= 64
>> $4 ==> true
>
> Yes, inserting explicit casts seems less clean than changing `codeLengthOf`
> to this:
>
> private static int codeLengthOf(char c) {
> return (int) (codes[c] & 0xL);
> }
>
> There are 256 elements in constant `long[] codes`. One could easily check
> that each element when ANDed with `0xL` results in a value
> that fits into the first 31 bits of `int`.
OK - I will change codeLengthOf as suggested. It was not immediately obvious
to me that the values would fit in the first 31 bits.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 18:42:46 GMT, Daniel Fuchs wrote:
>> In relation to
>> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find
>> here a patch that addresses possibly lossy conversions in java.net.http
>
> Daniel Fuchs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert adding char constants
src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line
291:
> 289:
> 290: HeaderWriter noMask() {
> 291: // The negation "~" sets the high order bits
Rubber-stamping this in front of every of the four closely sitting casts seems
excessive:
// The negation "~" sets the high order bits
// so the value is more than 16 bits and the
// compiler will emit a warning if not cast
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 08:56:26 GMT, Daniel Fuchs wrote:
>> No because the int returned could be negative, while the long will not.
>> Assuming bufferLen is 0 and codeLengthOf() returns some value that has the
>> 32th bit set to 1 then when codeLengthOf() returns long, bufferLen +
>> codeLengthOf() will be a positive long > 64, and we won't enter the `if`
>> here but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf()
>> would be negative and the `if` would be wrongly entered. I am not 100% sure
>> this is a scenario that might occur (codeLengthOf() returning large
>> "unsigned int" values) - but I'd prefer to stay on the safe side and assume
>> that it can.
>
> This is what I mean:
>
> jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1
> codeLengthOf ==> 2147483648
>
> jshell> int bufferLen = 0
> bufferLen ==> 0
>
> jshell> bufferLen + codeLengthOf <= 64
> $3 ==> false
>
> jshell> bufferLen + (int)codeLengthOf <= 64
> $4 ==> true
Yes, inserting explicit casts seems less clean than changing `codeLengthOf` to
this:
private static int codeLengthOf(char c) {
return (int) (codes[c] & 0xL);
}
There are 256 elements in constant `long[] codes`. One could easily check that
each element when ANDed with `0xL` results in a value that fits
into the first 31 bits of `int`.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 08:42:39 GMT, Daniel Fuchs wrote: >> codeLengthOf() returns long. It could be changed to return int with a cast >> internally and then you could avoid the two new casts. > > No because the int returned could be negative, while the long will not. > Assuming bufferLen is 0 and codeLengthOf() returns some value that has the > 32th bit set to 1 then when codeLengthOf() returns long, bufferLen + > codeLengthOf() will be a positive long > 64, and we won't enter the `if` here > but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf() would be > negative and the `if` would be wrongly entered. I am not 100% sure this is a > scenario that might occur (codeLengthOf() returning large "unsigned int" > values) - but I'd prefer to stay on the safe side and assume that it can. This is what I mean: jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1 codeLengthOf ==> 2147483648 jshell> int bufferLen = 0 bufferLen ==> 0 jshell> bufferLen + codeLengthOf <= 64 $3 ==> false jshell> bufferLen + (int)codeLengthOf <= 64 $4 ==> true - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 20:31:00 GMT, Michael McMahon wrote: >> I believe the method returns an "unsigned int" - having the method return an >> int would then potentially cause `bufferLen + len <= 64` to yield true when >> it shouldn't. Hopefully @pavelrappo will comment. > > codeLengthOf() returns long. It could be changed to return int with a cast > internally and then you could avoid the two new casts. No because the int returned could be negative, while the long will not. Assuming bufferLen is 0 and codeLengthOf() returns some value that has the 32th bit set to 1 then when codeLengthOf() returns long, bufferLen + codeLengthOf() will be a positive long > 64, and we won't enter the `if` here but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf() would be negative and the `if` would be wrongly entered. I am not 100% sure this is a scenario that might occur (codeLengthOf() returning large "unsigned int" values) - but I'd prefer to stay on the safe side and assume that it can. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 17:10:45 GMT, Daniel Fuchs wrote: >> src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java >> line 739: >> >>> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >>> append >>> 738: bufferLen += (int) len; >>> 739: pos++; >> >> Would it be cleaner to do the cast in the codeLengthOf method, so it returns >> an int? > > I believe the method returns an "unsigned int" - having the method return an > int would then potentially cause `bufferLen + len <= 64` to yield true when > it shouldn't. Hopefully @pavelrappo will comment. codeLengthOf() returns long. It could be changed to return int with a cast internally and then you could avoid the two new casts. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/fbf3c9a1..2334b747 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=03-04 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v4]
On Wed, 11 May 2022 18:25:00 GMT, Daniel Fuchs wrote: >> @RogerRiggs Actually - I'm no longer sure that this will work: >> >> >> jshell> char x = 0b1000_ >> x ==> '耀' >> >> jshell> var y = ~x >> y ==> -32769 >> >> jshell> char y = ~x >> | Error: >> | incompatible types: possible lossy conversion from int to char >> | char y = ~x; >> | ^^ > > So if x is a char, ~x seems to be an int :-( I have reverted adding constant fields. Too bad - the casts are back. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v4]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/ec344eef..fbf3c9a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=02-03 Stats: 29 lines in 1 file changed: 15 ins; 3 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
On Wed, 11 May 2022 18:23:30 GMT, Daniel Fuchs wrote: >> I'd put `_MASK` in the name along with whatever name is used for the bits >> in the frame spec . > > @RogerRiggs Actually - I'm no longer sure that this will work: > > > jshell> char x = 0b1000_ > x ==> '耀' > > jshell> var y = ~x > y ==> -32769 > > jshell> char y = ~x > | Error: > | incompatible types: possible lossy conversion from int to char > | char y = ~x; > | ^^ So if x is a char, ~x seems to be an int :-( - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
On Wed, 11 May 2022 17:49:28 GMT, Roger Riggs wrote: >> Ah! Good point. Maybe I should use a constant and get rid of the cast. > > I'd put `_MASK` in the name along with whatever name is used for the bits > in the frame spec . @RogerRiggs Actually - I'm no longer sure that this will work: jshell> char x = 0b1000_ x ==> '耀' jshell> var y = ~x y ==> -32769 jshell> char y = ~x | Error: | incompatible types: possible lossy conversion from int to char | char y = ~x; | ^^ - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Add _MASK suffix to char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/bbcf238b..ec344eef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=01-02 Stats: 14 lines in 1 file changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
On Wed, 11 May 2022 17:49:28 GMT, Roger Riggs wrote: >> Ah! Good point. Maybe I should use a constant and get rid of the cast. > > I'd put `_MASK` in the name along with whatever name is used for the bits > in the frame spec . Done - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
On Wed, 11 May 2022 18:02:32 GMT, Daniel Fuchs wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Use char constant to avoid casts LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Use char constant to avoid casts - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/6ef906e0..bbcf238b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=00-01 Stats: 29 lines in 1 file changed: 5 ins; 15 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:37:44 GMT, Roger Riggs wrote:
>> Yes - that's my understanding.
>
> These methods either set or clear a single bit in `firstChar`.
> The constant is an `int` so its complement is a 32 bit int.
>
> It could also be written as `~ (char) 0b100_000`. Then the 16 bit
> unsigned char would be complemented.
> Either way, the cast is needed to be explicit about the size of the constant.
>
> Another way to avoid the cast would be to define the bit positions as:
>
> `private static final char FIN_BIT = 0b1000_;
> ... etc.
> `
> Then the code in the methods would not need the cast.
>
>
> if (value) {
> firstChar |= FIN_BIT;
> } else {
> firstChar &= ~FIN_BIT;
> }
Ah! Good point. Maybe I should use a constant and get rid of the cast.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:46:15 GMT, Daniel Fuchs wrote:
>> These methods either set or clear a single bit in `firstChar`.
>> The constant is an `int` so its complement is a 32 bit int.
>>
>> It could also be written as `~ (char) 0b100_000`. Then the 16 bit
>> unsigned char would be complemented.
>> Either way, the cast is needed to be explicit about the size of the constant.
>>
>> Another way to avoid the cast would be to define the bit positions as:
>>
>> `private static final char FIN_BIT = 0b1000_;
>> ... etc.
>> `
>> Then the code in the methods would not need the cast.
>>
>>
>> if (value) {
>> firstChar |= FIN_BIT;
>> } else {
>> firstChar &= ~FIN_BIT;
>> }
>
> Ah! Good point. Maybe I should use a constant and get rid of the cast.
I'd put `_MASK` in the name along with whatever name is used for the bits in
the frame spec .
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:04:19 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
>> line 222:
>>
>>> 220: // compiler will emit a warning if not cast
>>> 221: firstChar &= (char) ~0b1000_;
>>> 222: }
>>
>> Am I right in believing that there was an implicit cast to char here before
>> and the only change is to make it explicit? ie. there is no actual behavior
>> change?
>
> Yes - that's my understanding.
These methods either set or clear a single bit in `firstChar`.
The constant is an `int` so its complement is a 32 bit int.
It could also be written as `~ (char) 0b100_000`. Then the 16 bit
unsigned char would be complemented.
Either way, the cast is needed to be explicit about the size of the constant.
Another way to avoid the cast would be to define the bit positions as:
`private static final char FIN_BIT = 0b1000_;
... etc.
`
Then the code in the methods would not need the cast.
if (value) {
firstChar |= FIN_BIT;
} else {
firstChar &= ~FIN_BIT;
}
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 16:52:07 GMT, Michael McMahon wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java > line 739: > >> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >> append >> 738: bufferLen += (int) len; >> 739: pos++; > > Would it be cleaner to do the cast in the codeLengthOf method, so it returns > an int? I believe the method returns an "unsigned int" - having the method return an int would then potentially cause `bufferLen + len <= 64` to yield true when it shouldn't. Hopefully @pavelrappo will comment. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 16:55:16 GMT, Michael McMahon wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java > line 222: > >> 220: // compiler will emit a warning if not cast >> 221: firstChar &= (char) ~0b1000_; >> 222: } > > Am I right in believing that there was an implicit cast to char here before > and the only change is to make it explicit? ie. there is no actual behavior > change? Yes - that's my understanding. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java line 739: > 737: buffer |= (codeValueOf(c) >>> bufferLen); // > append > 738: bufferLen += (int) len; > 739: pos++; Would it be cleaner to do the cast in the codeLengthOf method, so it returns an int? src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line 222: > 220: // compiler will emit a warning if not cast > 221: firstChar &= (char) ~0b1000_; > 222: } Am I right in believing that there was an implicit cast to char here before and the only change is to make it explicit? ie. there is no actual behavior change? - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http @pavelrappo I would appreciate if you could review these changes - PR: https://git.openjdk.java.net/jdk/pull/8656
RFR: 8286386: Address possibly lossy conversions in java.net.http
In relation to [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find here a patch that addresses possibly lossy conversions in java.net.http - Commit messages: - Fix comments - 8286386: Address possibly lossy conversions in java.net.http Changes: https://git.openjdk.java.net/jdk/pull/8656/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8656=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286386 Stats: 27 lines in 3 files changed: 15 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
