[PATCH] D116960: [ADT] Add an in-place version of toHex()

[Hans Wennborg via Phabricator via cfe-commits]

hans marked 2 inline comments as done.
hans added a comment.

In D116960#3232461 <https://reviews.llvm.org/D116960#3232461>, @dexonsmith wrote:

> Mostly LGTM, just a couple of comments on the drive-by changes to `hexdigit()` (I might split that (and the new call in toHex()) into a separate commit, but if you keep them here please mention them in the commit message).

Thanks! I'll commit the hexdigit() change separately.



================
Comment at: llvm/include/llvm/ADT/StringExtras.h:37
 inline char hexdigit(unsigned X, bool LowerCase = false) {
-  const char HexChar = LowerCase ? 'a' : 'A';
-  return X < 10 ? '0' + X : HexChar + X - 10;
+  assert(X <= 16);
+  static const char *const LUT = "0123456789ABCDEF";
----------------
dexonsmith wrote:
> Should this be `X < 16`?
D'oh, yes of course. Thanks for spotting that!


================
Comment at: llvm/include/llvm/ADT/StringExtras.h:38
+  assert(X <= 16);
+  static const char *const LUT = "0123456789ABCDEF";
+  const uint8_t Offset = LowerCase ? 32 : 0;
----------------
dexonsmith wrote:
> Can you use `constexpr StringLiteral` here?
Yes that would work, but I think it's simpler to just use the built-in types here.
Actually, there's no need for this to be a pointer, it should just be an array. I'll fix that.


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

https://reviews.llvm.org/D116960