[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3eed57e7ef7d: [ADT] Rename StringRef case insensitive 
methods for clarity (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D104819?vs=354178=354355#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

Files:
  llvm/include/llvm/ADT/StringRef.h
  llvm/lib/Support/StringRef.cpp
  llvm/unittests/ADT/StringRefTest.cpp

Index: llvm/unittests/ADT/StringRefTest.cpp
===
--- llvm/unittests/ADT/StringRefTest.cpp
+++ llvm/unittests/ADT/StringRefTest.cpp
@@ -98,15 +98,15 @@
   EXPECT_EQ( 1, StringRef("aab").compare("aa"));
   EXPECT_EQ( 1, StringRef("\xFF").compare("\1"));
 
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aAd"));
-  EXPECT_EQ( 0, StringRef("AaB").compare_lower("aab"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("AAA"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aaBb"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("bb"));
-  EXPECT_EQ( 1, StringRef("aaBb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("bb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("aA"));
-  EXPECT_EQ( 1, StringRef("\xFF").compare_lower("\1"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aAd"));
+  EXPECT_EQ( 0, StringRef("AaB").compare_insensitive("aab"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("AAA"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aaBb"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("bb"));
+  EXPECT_EQ( 1, StringRef("aaBb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("bb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("aA"));
+  EXPECT_EQ( 1, StringRef("\xFF").compare_insensitive("\1"));
 
   EXPECT_EQ(-1, StringRef("aab").compare_numeric("aad"));
   EXPECT_EQ( 0, StringRef("aab").compare_numeric("aab"));
@@ -366,14 +366,14 @@
   EXPECT_FALSE(Str.startswith("hi"));
 }
 
-TEST(StringRefTest, StartsWithLower) {
+TEST(StringRefTest, StartsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.startswith_lower(""));
-  EXPECT_TRUE(Str.startswith_lower("he"));
-  EXPECT_TRUE(Str.startswith_lower("hell"));
-  EXPECT_TRUE(Str.startswith_lower("HELlo"));
-  EXPECT_FALSE(Str.startswith_lower("helloworld"));
-  EXPECT_FALSE(Str.startswith_lower("hi"));
+  EXPECT_TRUE(Str.startswith_insensitive(""));
+  EXPECT_TRUE(Str.startswith_insensitive("he"));
+  EXPECT_TRUE(Str.startswith_insensitive("hell"));
+  EXPECT_TRUE(Str.startswith_insensitive("HELlo"));
+  EXPECT_FALSE(Str.startswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.startswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeFront) {
@@ -392,22 +392,22 @@
   EXPECT_TRUE(Str.consume_front(""));
 }
 
-TEST(StringRefTest, ConsumeFrontLower) {
+TEST(StringRefTest, ConsumeFrontInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
   EXPECT_EQ("heLLo", Str);
   EXPECT_FALSE(Str.consume_front("HEl"));
   EXPECT_EQ("heLLo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("HEl"));
+  EXPECT_TRUE(Str.consume_front_insensitive("HEl"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("loworld"));
+  EXPECT_FALSE(Str.consume_front_insensitive("loworld"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("ol"));
+  EXPECT_FALSE(Str.consume_front_insensitive("ol"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("lo"));
+  EXPECT_TRUE(Str.consume_front_insensitive("lo"));
   EXPECT_EQ("", Str);
-  EXPECT_FALSE(Str.consume_front_lower("o"));
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_FALSE(Str.consume_front_insensitive("o"));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
 }
 
 TEST(StringRefTest, EndsWith) {
@@ -419,14 +419,14 @@
   EXPECT_FALSE(Str.endswith("so"));
 }
 
-TEST(StringRefTest, EndsWithLower) {
+TEST(StringRefTest, EndsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.endswith_lower(""));
-  EXPECT_TRUE(Str.endswith_lower("lo"));
-  EXPECT_TRUE(Str.endswith_lower("LO"));
-  EXPECT_TRUE(Str.endswith_lower("ELlo"));
-  EXPECT_FALSE(Str.endswith_lower("helloworld"));
-  EXPECT_FALSE(Str.endswith_lower("hi"));
+  EXPECT_TRUE(Str.endswith_insensitive(""));
+  EXPECT_TRUE(Str.endswith_insensitive("lo"));
+  EXPECT_TRUE(Str.endswith_insensitive("LO"));
+  EXPECT_TRUE(Str.endswith_insensitive("ELlo"));
+  EXPECT_FALSE(Str.endswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.endswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeBack) {
@@ -445,22 +445,22 @@
   EXPECT_TRUE(Str.consume_back(""));
 }
 
-TEST(StringRefTest, ConsumeBackLower) {
+TEST(StringRefTest, ConsumeBackInsensitive) {
   StringRef Str("heLLo");
-  

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D104819#2839315 , @mstorsjo wrote:

> In D104819#2839263 , @dexonsmith 
> wrote:
>
>> Just be sure to clang-format when you do the mechanical changes in the 
>> follow up patches.)
>
> Hmm, those would have to be manually reviewed, but I guess I can try to do 
> that (discarding changes where the surrounding code isn't too well formatted 
> now already so it does more extensive reformattings other than for the 
> renaming).

Sounds good. (I'm generally in favour of erring on the side of trusting 
clang-format, especially for large refactorings like this; I wouldn't spend too 
much time analyzing whether it's an improvement...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D104819#2839263 , @dexonsmith 
wrote:

> LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.
>
> (I also see that there are some clang-format suggestions in the unit tests; 
> not sure any of them are actually improvements so maybe you left those out 
> intentionally?

Yeah those seem to be intentionally vertically aligned that way, no big opinion 
either way

> Just be sure to clang-format when you do the mechanical changes in the follow 
> up patches.)

Hmm, those would have to be manually reviewed, but I guess I can try to do that 
(discarding changes where the surrounding code isn't too well formatted now 
already so it does more extensive reformattings other than for the renaming).




Comment at: llvm/lib/Support/StringRef.cpp:37
 
-/// compare_lower - Compare strings, ignoring case.
-int StringRef::compare_lower(StringRef RHS) const {
+/// compare_insensitive - Compare strings, ignoring case.
+int StringRef::compare_insensitive(StringRef RHS) const {

dexonsmith wrote:
> You can drop these duplicate doxygen comments entirely from the `.cpp` file.
Sure, yeah doxygen in implementation files make little sense anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.

(I also see that there are some clang-format suggestions in the unit tests; not 
sure any of them are actually improvements so maybe you left those out 
intentionally? Just be sure to clang-format when you do the mechanical changes 
in the follow up patches.)




Comment at: llvm/include/llvm/ADT/StringRef.h:198
 
+LLVM_NODISCARD
+bool equals_lower(StringRef RHS) const { return equals_insensitive(RHS); }

You could probably drop the `LLVM_NODISCARD` from the soon-to-be-deleted 
`_lower` variants, but maybe it's not worth fussing with (up to you).



Comment at: llvm/lib/Support/StringRef.cpp:37
 
-/// compare_lower - Compare strings, ignoring case.
-int StringRef::compare_lower(StringRef RHS) const {
+/// compare_insensitive - Compare strings, ignoring case.
+int StringRef::compare_insensitive(StringRef RHS) const {

You can drop these duplicate doxygen comments entirely from the `.cpp` file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 354178.
mstorsjo added a comment.

Reduced this patch only to updating of the StringRef class itself and its unit 
test, removing the mechanical changes. Removed superfluous method name 
duplication in doxygen comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

Files:
  llvm/include/llvm/ADT/StringRef.h
  llvm/lib/Support/StringRef.cpp
  llvm/unittests/ADT/StringRefTest.cpp

Index: llvm/unittests/ADT/StringRefTest.cpp
===
--- llvm/unittests/ADT/StringRefTest.cpp
+++ llvm/unittests/ADT/StringRefTest.cpp
@@ -98,15 +98,15 @@
   EXPECT_EQ( 1, StringRef("aab").compare("aa"));
   EXPECT_EQ( 1, StringRef("\xFF").compare("\1"));
 
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aAd"));
-  EXPECT_EQ( 0, StringRef("AaB").compare_lower("aab"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("AAA"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("aaBb"));
-  EXPECT_EQ(-1, StringRef("AaB").compare_lower("bb"));
-  EXPECT_EQ( 1, StringRef("aaBb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("bb").compare_lower("AaB"));
-  EXPECT_EQ( 1, StringRef("AaB").compare_lower("aA"));
-  EXPECT_EQ( 1, StringRef("\xFF").compare_lower("\1"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aAd"));
+  EXPECT_EQ( 0, StringRef("AaB").compare_insensitive("aab"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("AAA"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("aaBb"));
+  EXPECT_EQ(-1, StringRef("AaB").compare_insensitive("bb"));
+  EXPECT_EQ( 1, StringRef("aaBb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("bb").compare_insensitive("AaB"));
+  EXPECT_EQ( 1, StringRef("AaB").compare_insensitive("aA"));
+  EXPECT_EQ( 1, StringRef("\xFF").compare_insensitive("\1"));
 
   EXPECT_EQ(-1, StringRef("aab").compare_numeric("aad"));
   EXPECT_EQ( 0, StringRef("aab").compare_numeric("aab"));
@@ -366,14 +366,14 @@
   EXPECT_FALSE(Str.startswith("hi"));
 }
 
-TEST(StringRefTest, StartsWithLower) {
+TEST(StringRefTest, StartsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.startswith_lower(""));
-  EXPECT_TRUE(Str.startswith_lower("he"));
-  EXPECT_TRUE(Str.startswith_lower("hell"));
-  EXPECT_TRUE(Str.startswith_lower("HELlo"));
-  EXPECT_FALSE(Str.startswith_lower("helloworld"));
-  EXPECT_FALSE(Str.startswith_lower("hi"));
+  EXPECT_TRUE(Str.startswith_insensitive(""));
+  EXPECT_TRUE(Str.startswith_insensitive("he"));
+  EXPECT_TRUE(Str.startswith_insensitive("hell"));
+  EXPECT_TRUE(Str.startswith_insensitive("HELlo"));
+  EXPECT_FALSE(Str.startswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.startswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeFront) {
@@ -392,22 +392,22 @@
   EXPECT_TRUE(Str.consume_front(""));
 }
 
-TEST(StringRefTest, ConsumeFrontLower) {
+TEST(StringRefTest, ConsumeFrontInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
   EXPECT_EQ("heLLo", Str);
   EXPECT_FALSE(Str.consume_front("HEl"));
   EXPECT_EQ("heLLo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("HEl"));
+  EXPECT_TRUE(Str.consume_front_insensitive("HEl"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("loworld"));
+  EXPECT_FALSE(Str.consume_front_insensitive("loworld"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_FALSE(Str.consume_front_lower("ol"));
+  EXPECT_FALSE(Str.consume_front_insensitive("ol"));
   EXPECT_EQ("Lo", Str);
-  EXPECT_TRUE(Str.consume_front_lower("lo"));
+  EXPECT_TRUE(Str.consume_front_insensitive("lo"));
   EXPECT_EQ("", Str);
-  EXPECT_FALSE(Str.consume_front_lower("o"));
-  EXPECT_TRUE(Str.consume_front_lower(""));
+  EXPECT_FALSE(Str.consume_front_insensitive("o"));
+  EXPECT_TRUE(Str.consume_front_insensitive(""));
 }
 
 TEST(StringRefTest, EndsWith) {
@@ -419,14 +419,14 @@
   EXPECT_FALSE(Str.endswith("so"));
 }
 
-TEST(StringRefTest, EndsWithLower) {
+TEST(StringRefTest, EndsWithInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.endswith_lower(""));
-  EXPECT_TRUE(Str.endswith_lower("lo"));
-  EXPECT_TRUE(Str.endswith_lower("LO"));
-  EXPECT_TRUE(Str.endswith_lower("ELlo"));
-  EXPECT_FALSE(Str.endswith_lower("helloworld"));
-  EXPECT_FALSE(Str.endswith_lower("hi"));
+  EXPECT_TRUE(Str.endswith_insensitive(""));
+  EXPECT_TRUE(Str.endswith_insensitive("lo"));
+  EXPECT_TRUE(Str.endswith_insensitive("LO"));
+  EXPECT_TRUE(Str.endswith_insensitive("ELlo"));
+  EXPECT_FALSE(Str.endswith_insensitive("helloworld"));
+  EXPECT_FALSE(Str.endswith_insensitive("hi"));
 }
 
 TEST(StringRefTest, ConsumeBack) {
@@ -445,22 +445,22 @@
   EXPECT_TRUE(Str.consume_back(""));
 }
 
-TEST(StringRefTest, ConsumeBackLower) {
+TEST(StringRefTest, ConsumeBackInsensitive) {
   StringRef Str("heLLo");
-  EXPECT_TRUE(Str.consume_back_lower(""));
+  

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D104819#2837421 , @dexonsmith 
wrote:

> Given how large this is, would it be reasonable to split this up a bit more?
>
> What I might do if this were my patch: get a review of the API change + the 
> manual changes in one patch (assuming there aren't many manual changes), then 
> land the remaining mechanical changes in chunks, perhaps vaguely by 
> component, likely using post-commit review. The benefit of committing by 
> chunks is that if there is some problem that comes up (even mechanical 
> changes fail) there's more granularity for bisection (and less churn on 
> revert). WDYT?

That sounds like a good plan to me, thanks for the suggestion!

> Also: is there a reference to this API in the ProgrammersManual? (I honestly 
> don't know, but if there is, please be sure to update it as well.)

I don't think so, at least grepping for `_lower` didn't hit anything in 
llvm/docs (and manually browsing it now, the section on StringRef doesn't 
mention those methods).




Comment at: llvm/include/llvm/ADT/StringRef.h:192
 
-/// equals_lower - Check for string equality, ignoring case.
+/// equals_insensitive - Check for string equality, ignoring case.
 LLVM_NODISCARD

MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
> 
> "Don’t duplicate function or class name at the beginning of the comment."
Thanks, I'll fix those for the methods I'm renaming here, but I'll refrain from 
touching the other methods in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: llvm/include/llvm/ADT/StringRef.h:192
 
-/// equals_lower - Check for string equality, ignoring case.
+/// equals_insensitive - Check for string equality, ignoring case.
 LLVM_NODISCARD

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

"Don’t duplicate function or class name at the beginning of the comment."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Given how large this is, would it be reasonable to split this up a bit more?

What I might do if this were my patch: get a review of the API change + the 
manual changes in one patch (assuming there aren't many manual changes), then 
land the remaining mechanical changes in chunks, perhaps vaguely by component, 
likely using post-commit review. The benefit of committing by chunks is that if 
there is some problem that comes up (even mechanical changes fail) there's more 
granularity for bisection (and less churn on revert). WDYT?

Also: is there a reference to this API in the ProgrammersManual? (I honestly 
don't know, but if there is, please be sure to update it as well.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits