[PATCH] D129224: [InstCombine] Fold strtoul and strtoull and avoid PR #56293

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:107
+  // Set Max to the maximum positive representable value in the type.
+  uint64_t Max = AsSigned && Negate;
+  Type *RetTy = CI->getType();
----------------
`uint64_t Max = AsSigned && Negate` is clever, but confusing at first glance; maybe just write out the conditional?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:145
+  uint64_t Result = 0;
+  for (const char *Dig = Str.data(); *Dig; ++Dig) {
+    unsigned char DigVal = *Dig;
----------------
This assumes the string is null-terminated?  I mean, we don't have to guarantee any particular behavior if it isn't, but we shouldn't read past the end of the string.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:147
+    unsigned char DigVal = *Dig;
+    if (isdigit(DigVal))
+      DigVal = DigVal - '0';
----------------
Please use llvm::isDigit etc. The C library routines interact with the current locale.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:164
+    unsigned long long PrevResult = Result;
+    Result = Result * Base + DigVal;
+
----------------
Maybe this would be easier to read using the SaturatingMultiplyAdd helper?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129224