[PATCH] Fix 'leading zeros' caveat of StringRef::compare_numeric()

[Sanne Wouda]

Improved readability by using strtol(), which handles the conversion to numericals.  It automatically handles prefixes of 0x for hexadecimal and 0 for octal.

Should be more readable than the original, as well. :)


REPOSITORY
  rL LLVM

http://reviews.llvm.org/D7388

Files:
  lib/Support/StringRef.cpp
  unittests/ADT/StringRefTest.cpp

Index: lib/Support/StringRef.cpp
===================================================================
--- lib/Support/StringRef.cpp
+++ lib/Support/StringRef.cpp
@@ -71,33 +71,51 @@
 
 /// compare_numeric - Compare strings, handle embedded numbers.
 int StringRef::compare_numeric(StringRef RHS) const {
-  for (size_t I = 0, E = std::min(Length, RHS.Length); I != E; ++I) {
+  // Point to the current characters to be compared.
+  const char *L = Data, *R = RHS.Data;
+
+  const char *L_end = L + Length;
+  const char *R_end = R + RHS.Length;
+
+  while (L < L_end && R < R_end) {
     // Check for sequences of digits.
-    if (ascii_isdigit(Data[I]) && ascii_isdigit(RHS.Data[I])) {
-      // The longer sequence of numbers is considered larger.
-      // This doesn't really handle prefixed zeros well.
-      size_t J;
-      for (J = I + 1; J != E + 1; ++J) {
-        bool ld = J < Length && ascii_isdigit(Data[J]);
-        bool rd = J < RHS.Length && ascii_isdigit(RHS.Data[J]);
-        if (ld != rd)
-          return rd ? -1 : 1;
-        if (!rd)
-          break;
+    if (ascii_isdigit(*L) && ascii_isdigit(*R)) {
+      char *new_L, *new_R;
+
+      // Try number conversions.
+      long left_num = strtol(L, &new_L, 0);
+      long right_num = strtol(R, &new_R, 0);
+
+      if (L == new_L || R == new_R) {
+        // One of the conversions failed.  Do a char-by-char comparison
+        // for the first character, and move on.
+        if (*L != *R)
+          return (unsigned)*L < (unsigned)*R ? -1 : 1;
+        L++; R++;
+        continue;
       }
-      // The two number sequences have the same length (J-I), just memcmp them.
-      if (int Res = compareMemory(Data + I, RHS.Data + I, J - I))
-        return Res < 0 ? -1 : 1;
-      // Identical number sequences, continue search after the numbers.
-      I = J - 1;
+
+      if (left_num != right_num)
+        return left_num < right_num ? -1 : 1;
+
+      // Continue where the numeric comparison ended.
+      L = new_L;
+      R = new_R;
       continue;
     }
-    if (Data[I] != RHS.Data[I])
-      return (unsigned char)Data[I] < (unsigned char)RHS.Data[I] ? -1 : 1;
+
+    // Do a char-by-char comparison, and move on.
+    if (*L != *R) {
+      return (unsigned)*L < (unsigned)*R ? -1 : 1;
+    }
+    L++; R++;
   }
-  if (Length == RHS.Length)
+
+  // Equal if we reached the end of both StringRefs.
+  if (L == L_end && R == R_end)
     return 0;
-  return Length < RHS.Length ? -1 : 1;
+
+  return L == L_end ? -1 : 1;
 }
 
 // Compute the edit distance between the two given strings.
Index: unittests/ADT/StringRefTest.cpp
===================================================================
--- unittests/ADT/StringRefTest.cpp
+++ unittests/ADT/StringRefTest.cpp
@@ -78,6 +78,11 @@
   EXPECT_EQ( 0, StringRef("10").compare_numeric("10"));
   EXPECT_EQ( 0, StringRef("10a").compare_numeric("10a"));
   EXPECT_EQ( 1, StringRef("2").compare_numeric("1"));
+  EXPECT_EQ(-1, StringRef("0a").compare_numeric("1"));
+  EXPECT_EQ( 1, StringRef("2").compare_numeric("01"));
+  EXPECT_EQ(-1, StringRef("2").compare_numeric("03"));
+  EXPECT_EQ( 0, StringRef("007 is 1337").compare_numeric("7 is 1337"));
+  EXPECT_EQ(-1, StringRef("010").compare_numeric("9"));
   EXPECT_EQ( 0, StringRef("llvm_v1i64_ty").compare_numeric("llvm_v1i64_ty"));
   EXPECT_EQ( 1, StringRef("\xFF").compare_numeric("\1"));
   EXPECT_EQ( 1, StringRef("V16").compare_numeric("V1_q0"));

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D7388.23274.patch
Type: text/x-patch
Size: 3447 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150406/19fc37cc/attachment.bin>