[PATCH] D24778: Add `StringRef::consumeInteger`

[Adrian McCarthy via llvm-commits]

amccarth added a subscriber: amccarth.

================
Comment at: llvm/trunk/include/llvm/ADT/StringRef.h:29
@@ -28,3 +28,3 @@
 
   /// Helper functions for StringRef::getAsInteger.
   bool getAsUnsignedInteger(StringRef Str, unsigned Radix,
----------------
Please indicate what true and false returns mean.  Consider making them private if these aren't the droids a general user is looking for.

================
Comment at: llvm/trunk/lib/Support/StringRef.cpp:364
@@ -363,3 +363,3 @@
 
   if (Str.startswith("0o")) {
     Str = Str.substr(2);
----------------
Probably outside the scope of this patch, but the proposal for `0o` for C++ allows the `O` to be uppercase as well, just as with the `X` and `B` for the other radixes.

================
Comment at: llvm/trunk/lib/Support/StringRef.cpp:384
@@ -384,3 +383,3 @@
   // Empty strings (after the radix autosense) are invalid.
   if (Str.empty()) return true;
 
----------------
`true` means invalid?!  Wow!  I did not see that coming.

I believe this check is unnecessary now, since this patch adds a check near the end to see if any characters were consumed.

================
Comment at: llvm/trunk/unittests/ADT/StringRefTest.cpp:581
@@ -580,3 +580,3 @@
   , "0o8"                   // illegal oct characters
   , "-123"                  // negative unsigned value
 };
----------------
Please add a case or two for a radix prefix followed by no valid digits.  For example:

`"0b", "0XQ"


Repository:
  rL LLVM

https://reviews.llvm.org/D24778