On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga <d...@openjdk.org> wrote:
> ### Summary > This change ensures we don't get undefined behavior when > calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). > `isspace` accepts an `int` argument that "the application shall ensure is a > character representable as an unsigned char or equal to the value of the > macro EOF.". > > Previously, there was no checking of the values passed to `isspace`. I've > replaced direct calls with a new wrapper `os::is_space` that performs a range > check and prevents the possibility of undefined behavior from happening. For > instances outside of Hotspot, I've added casts to `unsigned char`. > > **Testing** > - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check > `os::is_space` is working correctly. > - tier1 Thanks for the feedback everyone! > But I did a bit of searching and it seems it comes down to potential > arithmetic operations on the "char" the might behave differently depending on > the signed-ness. :( Ok, so it seems like we ought to cast to unsigned char whenever char is passed, even if the representable range is the same. One more argument in favor of the current wrapper approach is that it makes it easy to at-a-glance confirm `isspace` is being used correctly everywhere. However, I am fine with just using casts wherever there is char and removing the wrapper. This is simpler and also avoids the cast to int. But what about when an int is passed to `isspace`? Maybe having the range check here would be better than casting to unsigned char or leaving as is? ------------- PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2158401790
