llvm/ADT/StringExtras.h hexDigitValue - Init of integer buffer

[jerker.back]

Summary

Hi,

In the header StringExtras.h, function hexDigitValue

due to the expression

LUT[i] = -1U;

my compiler gives me

error C4146: unary minus operator applied to unsigned type, result still unsigned

This is a warning turned into an error by a default compiler switch
The patch gives one solution which I believe also could be more efficient

Thanks

[lebedev.ri]

llvm/include/llvm/ADT/StringExtras.h

70

This doesn't fill the entire array with -1
https://godbolt.org/z/G3Yqbh

74

How about just ~0U ?

[jerker.back]

How to submit a new patch?

llvm/include/llvm/ADT/StringExtras.h

74

Yes, of course

LUT[i] = ~0U;

works without the compiler complaining

[joerg]

Do we really want to change the code here? It is perfectly well defined behavior.

[jerker.back]

Expanding the patch to similar code places in the file.
Compiler warnings are gone under -Wall

[dexonsmith]

In D91968#2412092, @joerg wrote:

Do we really want to change the code here? It is perfectly well defined behavior.

I weakly agree with @joerg, I would have thought we have lots of cases of -1U since it's a common / well-understood way to get this value. Is the warning possible to turn off? Have you reported a compiler bug?

[jerker.back]

In D91968#2424469, @dexonsmith wrote:

In D91968#2412092, @joerg wrote:

Do we really want to change the code here? It is perfectly well defined behavior.

I weakly agree with @joerg, I would have thought we have lots of cases of -1U since it's a common / well-understood way to get this value. Is the warning possible to turn off? Have you reported a compiler bug?

As I said in the beginning, this is a compiler warning turned into an error by a default compiler option (-sdl, Security Development Lifecycle (SDL) checks). The option can be turned off. It's a bit unclear under what conditions exactly the option is default, off or disabled. The issue is well known and not a bug.

[dexonsmith]

In D91968#2425795, @jerker.back wrote:

In D91968#2424469, @dexonsmith wrote:

In D91968#2412092, @joerg wrote:

Do we really want to change the code here? It is perfectly well defined behavior.

I weakly agree with @joerg, I would have thought we have lots of cases of -1U since it's a common / well-understood way to get this value. Is the warning possible to turn off? Have you reported a compiler bug?

As I said in the beginning, this is a compiler warning turned into an error by a default compiler option (-sdl, Security Development Lifecycle (SDL) checks). The option can be turned off. It's a bit unclear under what conditions exactly the option is default, off or disabled. The issue is well known and not a bug.

Well, it seems like a bigger decision than just this patch, since there are 306 instances of this in llvm-project:

$ git grep -e -1U | wc -l
     306

I don't know if you were planning to change them all one-by-one, but it seems like a policy change if we're going to disallow this literal. Personally I find -1U more readable than ~0U and certainly easier to type (there are 121 of those, too, though).

I don't feel that strongly one way or the other, but this might deserve a discussion on llvm-dev.

[jerker.back]

In D91968#2427121, @dexonsmith wrote:

In D91968#2425795, @jerker.back wrote:

In D91968#2424469, @dexonsmith wrote:

In D91968#2412092, @joerg wrote:

Do we really want to change the code here? It is perfectly well defined behavior.

I weakly agree with @joerg, I would have thought we have lots of cases of -1U since it's a common / well-understood way to get this value. Is the warning possible to turn off? Have you reported a compiler bug?

As I said in the beginning, this is a compiler warning turned into an error by a default compiler option (-sdl, Security Development Lifecycle (SDL) checks). The option can be turned off. It's a bit unclear under what conditions exactly the option is default, off or disabled. The issue is well known and not a bug.

Well, it seems like a bigger decision than just this patch, since there are 306 instances of this in llvm-project:

$ git grep -e -1U | wc -l
     306

I don't know if you were planning to change them all one-by-one, but it seems like a policy change if we're going to disallow this literal. Personally I find -1U more readable than ~0U and certainly easier to type (there are 121 of those, too, though).

I don't feel that strongly one way or the other, but this might deserve a discussion on llvm-dev.

Thanks for the effort to investigate and I agree fully with you. Since all the other occurrences throw a warning as well I'm aware of the extent. My immediate interests lie to the PDB debug info code, which is based on Microsoft code at GitHub, which in turn I think must have been published just for the need in LLVM. It's not easy to break out functionality like the debug info handling from LLVM and other dependencies follow, especially headers.

llvm/include/llvm/ADT/StringExtras.h

     constexpr HexTable() {
       // Default initialize everything to invalid.
       for (int i = 0; i < 255; ++i)
-        LUT[i] = -1U;
+        LUT[i] = ~0U;
       // Initialize `0`-`9`.
       for (int i = 0; i < 10; ++i)
         LUT['0' + i] = i;

 inline bool isDigit(char C) { return C >= '0' && C <= '9'; }
 
 /// Checks if character \p C is a hexadecimal numeric character.
-inline bool isHexDigit(char C) { return hexDigitValue(C) != -1U; }
+inline bool isHexDigit(char C) { return hexDigitValue(C) != ~0U; }
 
 /// Checks if character \p C is a valid letter as classified by "C" locale.
 inline bool isAlpha(char C) {

 inline bool tryGetHexFromNibbles(char MSB, char LSB, uint8_t &Hex) {
   unsigned U1 = hexDigitValue(MSB);
   unsigned U2 = hexDigitValue(LSB);
-  if (U1 == -1U || U2 == -1U)
+  if (U1 == ~0U || U2 == ~0U)
     return false;
 
   Hex = static_cast<uint8_t>((U1 << 4) | U2);