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

[Martin Sebor via Phabricator via llvm-commits]

msebor marked an inline comment as done.
msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:91
+  // Strip leading whitespace.
+  for (unsigned i = 0; i != Str.size(); ++i)
+    if (!isSpace((unsigned char)Str[i])) {
----------------
yurai007 wrote:
> Couldn't it be simplified a bit to something like: Str = Str.drop_while([](char c) { return isSpace(c); }); ?
Yes, it could be rewritten like that.  I'm not sure everyone will find the more compact form as readable.  Not every compiler also inlines the lambda invocation (Clang does but GCC doesn't, likely due to exceeding some inlining limit), so there may be a runtime cost to pay.  On balance I'm not sure it matters enough one way or the other so unless there's a strong preference for the lambda I'd just as soon leave it as is.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:120
+    Max += AsSigned ? INT64_MAX : UINT64_MAX;
+    break;
+  default:
----------------
nikic wrote:
> Can avoid the separate cases via `AsSigned ? maxUIntN(NBits) : maxIntN(NBits)`?
Yes, that's simpler, thanks.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:127
+    // Autodetect Base.
+    if (Str.size() > 1) {
+      if (Str[0] == '0') {
----------------
yurai007 wrote:
> Couldn't it be simplified a bit to something like:
> 
> if (Str.startswith_insensitive("0x")  {
> Base = 16;
> Str = Str.drop_front(2);
> } else {...}
> 
> ?
Thanks for the suggestion.  The code tests for `'0'` first, before testing for `'x'`, and I'd rather keep it that way than repeating the former if the `"0x"` test fails.


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

https://reviews.llvm.org/D129224