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

[Martin Sebor via Phabricator via llvm-commits]

msebor added a comment.

Ping: @efriedma, please let me know if you have any further comments or suggestions, or if the last revisions is good enough to commit.  Thanks!



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:115
+
+  // Set Max to the maximum positive representable value in the type.
+  Type *RetTy = CI->getType();
----------------
barannikov88 wrote:
> This comment is not true for the case when AsSigned and Negate are both true. I.e. 0x80 is not the maximum possible representable value in i8 type. Something like "maximum possible value of the magnitude of a number which could be encoded" might be better.
> Excuse my English
I agree that //magnitude// might be more accurate, although to me the suggested rewording sounds too cumbersome to be an overall improvement.  I'll see if I can work the word into the comment without making it harder to understand.  Thanks!


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:119
+  uint64_t Max = AsSigned && Negate ? 1 : 0;
+  Max += AsSigned ? maxIntN(NBits) : maxUIntN(NBits);
+
----------------
barannikov88 wrote:
> Honestly, I'd prefer more verbose code to this bit manipulation (it took me two minutes to figure out that the added 1 deals with two's complement representation). E.g.
> ```
> if (AsSigned)
>   Max = Negate ? -(uint64_t)minIntN(NBits) : maxIntN(NBits)
> else
>   Max = maxUIntN(NBits);
> ```
> or, if you prefer one-liners:
> `Max = AsSigned ? Negate ? -(uint64_t)minIntN(NBits) : maxIntN(NBits) : maxUIntN(NBits)`
> 
I don't see either of the suggested forms as an improvement, although (obviously) others might feel differently.  Since it's a matter of taste I'm going to leave it as is.


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

https://reviews.llvm.org/D129224