[PATCH] D91968: llvm/ADT/StringExtras.h hexDigitValue - Init of integer buffer

[Jerker Bäck via Phabricator via llvm-commits]

jerker.back added a comment.

In D91968#2427121 <https://reviews.llvm.org/D91968#2427121>, @dexonsmith wrote:

> In D91968#2425795 <https://reviews.llvm.org/D91968#2425795>, @jerker.back wrote:
>
>> In D91968#2424469 <https://reviews.llvm.org/D91968#2424469>, @dexonsmith wrote:
>>
>>> In D91968#2412092 <https://reviews.llvm.org/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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91968/new/

https://reviews.llvm.org/D91968