[ADT] Add StringRef consume_front_lower and consume_back_lower

[mstorsjo]

Summary

These serve as a convenient combination of consume_front/back and
startswith_lower/endswith_lower.

[MaskRay]

Can you show a patch which will benefit from this? Otherwise this looks good.

[lattner]

I don't think this is general enough requirement to add a specialized method for it. Did you consider adding a consumes_front/back overload that takes a lambda?

[mstorsjo]

In D104218#2817946, @MaskRay wrote:

Can you show a patch which will benefit from this? Otherwise this looks good.

I have one existing use with a local helper function in llvm-rc here: https://github.com/llvm/llvm-project/blob/d8c5a4d6b6efad405c71ead8997276d8d3a7c5ad/llvm/tools/llvm-rc/llvm-rc.cpp#L259-L277 And I want to use it similarly in D104212. (llvm-ar does case insensitive switching between different modes of the same binary based on argv[0] this way, https://github.com/llvm/llvm-project/blob/d8c5a4d6b6efad405c71ead8997276d8d3a7c5ad/llvm/tools/llvm-ar/llvm-ar.cpp#L1273-L1291, so therefore I suppose it's good to keep the rest of the parsing of argv[0] similarly case insensitive.)

In D104218#2818302, @lattner wrote:

I don't think this is general enough requirement to add a specialized method for it. Did you consider adding a consumes_front/back overload that takes a lambda?

I didn't, do you mean something like this?

bool consume_front(function_ref<bool(StringRef)> F) {
  if (!F(*this))
    return false;

  *this = drop_front(/*how much?*/);
  return true;
}

Then the info about how much to drop has to be passed separately somehow, maybe by having the lambda return a std::pair<bool, size_t>? I guess that'd work and keep it generic, but it loses all the convenience of the function in such string parsing code - right now it's pretty straightforward to write compact and efficient parsing code with the other StringRef functions.

[lattner]

I was thinking that the closure would take a character, not a stringref. So you could do something like str.consume_back(islower).

[mstorsjo]

In D104218#2819837, @lattner wrote:

I was thinking that the closure would take a character, not a stringref. So you could do something like str.consume_back(islower).

It's not about whether characters are lowercase or not, but whether the string ends with the given pattern, ignoring case. The _lower suffix is maybe a bit of a misnomer here, as it's not about lowercase itself but about case insensitivity, but it follows the existing pattern of endswith_lower() and startswith_lower().

[lattner]

Oh, I completely missed that. I would recommend using the word case_insensitive here. I don't think stringref has any other case insensitive support, and I don't think this is the obvious place to start. I would start with case insensitive equality comparison. Your client can use that as a primitive that it builds higher level functionality on top of.

-Chris

[mstorsjo]

In D104218#2820315, @lattner wrote:

Oh, I completely missed that. I would recommend using the word case_insensitive here. I don't think stringref has any other case insensitive support, and I don't think this is the obvious place to start. I would start with case insensitive equality comparison. Your client can use that as a primitive that it builds higher level functionality on top of.

StringRef does already have a bunch of case insensitive comparisons, and those are already used as primitives that this new method builds on. If it didn't, then clearly this wouldn't be the place to start, but it already has almost complete coverage of case insensitive versions of methods. E.g. equals_lower, compare_lower, startswith_lower, endswith_lower, find_lower, rfind_lower, contains_lower. This patch mostly just fills in a gap in the coverage of the existing case insensitive methods. So therefore I'd prefer to add this method to the same naming scheme, even if it isn't entirely obvious (this isn't the place to start deviating from the established pattern).

As consume_back is a convenience wrapper over endswith and drop_back, I'm suggesting to add consume_back_lower which builds on endswith_lower (and drop_back).

[mstorsjo]

Squashed in two changes to two tools that can get rid of their own private helper function if this is hoisted to StringRef.

[mstorsjo]

@lattner Do you want to follow up on this one? StringRef already has a bunch of case insensitive operations, and this patch just fills a gap in the set of those operations.

[lattner]

Aha, I see this is consistent with the existing naming convention for this, so I'm ok with it.

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

[mstorsjo]

In D104218#2832251, @lattner wrote:

Aha, I see this is consistent with the existing naming convention for this, so I'm ok with it.

Thanks!

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

I see that the discussion thread from last year regarding C++ API (lack of) stability didn't end up into something that was committed (https://lists.llvm.org/pipermail/llvm-dev/2020-July/143613.html); changing ADT APIs can probably cause a fair bit of churn for e.g. out of tree users. Then again, I guess these particular methods are a bit less frequently used than the rest of ADT, so it's maybe not a biggie? Or should we be kind and do it as a two-stage change, adding new methods with new names and flagging the old ones as deprecated, and removing the deprecated ones in a week or two?

[foad]

Or should we be kind and do it as a two-stage change, adding new methods with new names and flagging the old ones as deprecated, and removing the deprecated ones in a week or two?

Drive-by comment from a downstream user: I would appreciate you doing it as a two-stage change but there is no need to wait a week or two in between. All I want is a point in git history that I can sync to, that has both the old and new APIs available.

[mstorsjo]

In D104218#2832592, @foad wrote:

Or should we be kind and do it as a two-stage change, adding new methods with new names and flagging the old ones as deprecated, and removing the deprecated ones in a week or two?

Drive-by comment from a downstream user: I would appreciate you doing it as a two-stage change but there is no need to wait a week or two in between. All I want is a point in git history that I can sync to, that has both the old and new APIs available.

Thanks for the input! Btw, if you happen to be able to check easily in the diff between your downstream fork vs upstream, do you happen to have a lot use of these _lower() method calls, outside of code in upstream? (A quick grep in llvm-project shows about only half a dozen per target in llvm/lib/Targets, but a fair bit more in lldb/lld/clang.)

[foad]

Btw, if you happen to be able to check easily in the diff between your downstream fork vs upstream, do you happen to have a lot use of these _lower() method calls, outside of code in upstream?

No, none actually! This was more of a general comment about how to do incompatible changes in a downstream-friendly way.

[dexonsmith]

In D104218#2832463, @mstorsjo wrote:

In D104218#2832251, @lattner wrote:

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

A few ideas to consider:

• X_casei()
• X_nocase()
• X_ignorecase()
• X_foldcase() (term from text search)

(Looks like X_lower() has been with us for over a decade (68e4945c03eeeb04773587f3b265b0888bb22549 / r87020), but I think equal_lower and compare_lower were less ambiguous than the newer ones.)

In D104218#2832677, @foad wrote:

Btw, if you happen to be able to check easily in the diff between your downstream fork vs upstream, do you happen to have a lot use of these _lower() method calls, outside of code in upstream?

No, none actually! This was more of a general comment about how to do incompatible changes in a downstream-friendly way.

Right, no real need for a buffer in time. Once the change is split in two, it's easy for downstreams to temporarily revert the second commit while they update out-of-tree code at their own pace.

[lattner]

I'd recommend you use "insensitive" instead of "lower", e.g. consume_front_insensitive.

[mstorsjo]

In D104218#2833781, @dexonsmith wrote:

In D104218#2832463, @mstorsjo wrote:

In D104218#2832251, @lattner wrote:

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

A few ideas to consider:

• X_casei()
• X_nocase()
• X_ignorecase()
• X_foldcase() (term from text search)

Thanks for the suggestions! What do you think of @lattner ‘s suggestion (X_insensitive)?

[dexonsmith]

In D104218#2836398, @mstorsjo wrote:

In D104218#2833781, @dexonsmith wrote:

In D104218#2832463, @mstorsjo wrote:

In D104218#2832251, @lattner wrote:

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

A few ideas to consider:

• X_casei()
• X_nocase()
• X_ignorecase()
• X_foldcase() (term from text search)

Thanks for the suggestions! What do you think of @lattner ‘s suggestion (X_insensitive)?

SGTM!

llvm/include/llvm/ADT/StringRef.h

#endif
     bool consume_front(StringRef Prefix) {
       if (!startswith(Prefix))
         return false;
 
       *this = drop_front(Prefix.size());
       return true;
     }
 
+    /// Returns true if this StringRef has the given prefix, ignoring case,
+    /// and removes that prefix.
+    bool consume_front_lower(StringRef Prefix) {
+      if (!startswith_lower(Prefix))
+        return false;
+
+      *this = drop_front(Prefix.size());
+      return true;
+    }
+
     /// Returns true if this StringRef has the given suffix and removes that
     /// suffix.
     bool consume_back(StringRef Suffix) {
       if (!endswith(Suffix))
         return false;
 
       *this = drop_back(Suffix.size());
       return true;
     }
 
+    /// Returns true if this StringRef has the given suffix, ignoring case,
+    /// and removes that suffix.
+    bool consume_back_lower(StringRef Suffix) {
+      if (!endswith_lower(Suffix))
+        return false;
+
+      *this = drop_back(Suffix.size());
+      return true;
+    }
+
     /// Return a reference to the substring from [Start, End).
     ///
     /// \param Start The index of the starting character in the substring; if
     /// the index is npos or greater than the length of the string then the
     /// empty substring will be returned.
     ///
     /// \param End The index following the last character to include in the
     /// substring. If this is npos or exceeds the number of characters

llvm/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp

default:
     return COFF::IMAGE_FILE_MACHINE_UNKNOWN;
   }
 }
 
 MachineTypes getDefaultMachine() {
   return getMachine(Triple(sys::getDefaultTargetTriple()));
 }
 
-static bool consume_back_lower(StringRef &S, const char *Str) {
-  if (!S.endswith_lower(Str))
-    return false;
-  S = S.drop_back(strlen(Str));
-  return true;
-}
-
 Optional<std::string> getPrefix(StringRef Argv0) {
   StringRef ProgName = llvm::sys::path::stem(Argv0);
   // x86_64-w64-mingw32-dlltool -> x86_64-w64-mingw32
   // llvm-dlltool -> None
   // aarch64-w64-mingw32-llvm-dlltool-10.exe -> aarch64-w64-mingw32
   ProgName = ProgName.rtrim("0123456789.-");
-  if (!consume_back_lower(ProgName, "dlltool"))
+  if (!ProgName.consume_back_lower("dlltool"))
     return None;
-  consume_back_lower(ProgName, "llvm-");
-  consume_back_lower(ProgName, "-");
+  ProgName.consume_back_lower("llvm-");
+  ProgName.consume_back_lower("-");
   return ProgName.str();
 }
 
 } // namespace
 
 int llvm::dlltoolDriverMain(llvm::ArrayRef<const char *> ArgsArr) {
   DllOptTable Table;
   unsigned MissingIndex;

llvm/tools/llvm-rc/llvm-rc.cpp

bool preprocess(StringRef Src, StringRef Dst, const RcOptions &Opts,
   // process; otherwise we could avoid using a temp file.
   int Res = sys::ExecuteAndWait(Clang, Args);
   if (Res) {
     fatalError("llvm-rc: Preprocessing failed.");
   }
   return true;
 }
 
-static bool consume_back_lower(StringRef &S, const char *Str) {
-  if (!S.endswith_lower(Str))
-    return false;
-  S = S.drop_back(strlen(Str));
-  return true;
-}
-
 static std::pair<bool, std::string> isWindres(llvm::StringRef Argv0) {
   StringRef ProgName = llvm::sys::path::stem(Argv0);
   // x86_64-w64-mingw32-windres -> x86_64-w64-mingw32, windres
   // llvm-rc -> "", llvm-rc
   // aarch64-w64-mingw32-llvm-windres-10.exe -> aarch64-w64-mingw32, llvm-windres
   ProgName = ProgName.rtrim("0123456789.-");
-  if (!consume_back_lower(ProgName, "windres"))
+  if (!ProgName.consume_back_lower("windres"))
     return std::make_pair<bool, std::string>(false, "");
-  consume_back_lower(ProgName, "llvm-");
-  consume_back_lower(ProgName, "-");
+  ProgName.consume_back_lower("llvm-");
+  ProgName.consume_back_lower("-");
   return std::make_pair<bool, std::string>(true, ProgName.str());
 }
 
 Format parseFormat(StringRef S) {
   Format F = StringSwitch<Format>(S.lower())
                  .Case("rc", Rc)
                  .Case("res", Res)
                  .Case("coff", Coff)

llvm/unittests/ADT/StringRefTest.cpp

TEST(StringRefTest, ConsumeFront) {
   EXPECT_FALSE(Str.consume_front("lol"));
   EXPECT_EQ("llo", Str);
   EXPECT_TRUE(Str.consume_front("llo"));
   EXPECT_EQ("", Str);
   EXPECT_FALSE(Str.consume_front("o"));
   EXPECT_TRUE(Str.consume_front(""));
 }
 
+TEST(StringRefTest, ConsumeFrontLower) {
+  StringRef Str("heLLo");
+  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_EQ("heLLo", Str);
+  EXPECT_FALSE(Str.consume_front("HEl"));
+  EXPECT_EQ("heLLo", Str);
+  EXPECT_TRUE(Str.consume_front_lower("HEl"));
+  EXPECT_EQ("Lo", Str);
+  EXPECT_FALSE(Str.consume_front_lower("loworld"));
+  EXPECT_EQ("Lo", Str);
+  EXPECT_FALSE(Str.consume_front_lower("ol"));
+  EXPECT_EQ("Lo", Str);
+  EXPECT_TRUE(Str.consume_front_lower("lo"));
+  EXPECT_EQ("", Str);
+  EXPECT_FALSE(Str.consume_front_lower("o"));
+  EXPECT_TRUE(Str.consume_front_lower(""));
+}
+
 TEST(StringRefTest, EndsWith) {
   StringRef Str("hello");
   EXPECT_TRUE(Str.endswith(""));
   EXPECT_TRUE(Str.endswith("lo"));
   EXPECT_FALSE(Str.endswith("helloworld"));
   EXPECT_FALSE(Str.endswith("worldhello"));
   EXPECT_FALSE(Str.endswith("so"));
 }
TEST(StringRefTest, ConsumeBack) {
   EXPECT_FALSE(Str.consume_back("hle"));
   EXPECT_EQ("hel", Str);
   EXPECT_TRUE(Str.consume_back("hel"));
   EXPECT_EQ("", Str);
   EXPECT_FALSE(Str.consume_back("h"));
   EXPECT_TRUE(Str.consume_back(""));
 }
 
+TEST(StringRefTest, ConsumeBackLower) {
+  StringRef Str("heLLo");
+  EXPECT_TRUE(Str.consume_back_lower(""));
+  EXPECT_EQ("heLLo", Str);
+  EXPECT_FALSE(Str.consume_back("lO"));
+  EXPECT_EQ("heLLo", Str);
+  EXPECT_TRUE(Str.consume_back_lower("lO"));
+  EXPECT_EQ("heL", Str);
+  EXPECT_FALSE(Str.consume_back_lower("helhel"));
+  EXPECT_EQ("heL", Str);
+  EXPECT_FALSE(Str.consume_back_lower("hle"));
+  EXPECT_EQ("heL", Str);
+  EXPECT_TRUE(Str.consume_back_lower("hEl"));
+  EXPECT_EQ("", Str);
+  EXPECT_FALSE(Str.consume_back_lower("h"));
+  EXPECT_TRUE(Str.consume_back_lower(""));
+}
+
 TEST(StringRefTest, Find) {
   StringRef Str("helloHELLO");
   StringRef LongStr("hellx xello hell ello world foo bar hello HELLO");
 
   struct {
     StringRef Str;
     char C;
     std::size_t From;