[clang-format][NFC] Replace a function with StringRef::contains

[owenca]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/146245.diff

1 Files Affected:

• (modified) clang/lib/Format/BreakableToken.cpp (+1-13)

diff --git a/clang/lib/Format/BreakableToken.cpp b/clang/lib/Format/BreakableToken.cpp
index def0d73e77539..24912c25ef8c6 100644
--- a/clang/lib/Format/BreakableToken.cpp
+++ b/clang/lib/Format/BreakableToken.cpp
@@ -26,18 +26,6 @@ namespace clang {
 namespace format {
 
 static constexpr StringRef Blanks = " \t\v\f\r";
-static bool IsBlank(char C) {
-  switch (C) {
-  case ' ':
-  case '\t':
-  case '\v':
-  case '\f':
-  case '\r':
-    return true;
-  default:
-    return false;
-  }
-}
 
 static StringRef getLineCommentIndentPrefix(StringRef Comment,
                                             const FormatStyle &Style) {
@@ -193,7 +181,7 @@ getStringSplit(StringRef Text, unsigned UsedColumns, unsigned ColumnLimit,
     if (Chars > MaxSplit || Text.size() <= Advance)
       break;
 
-    if (IsBlank(Text[0]))
+    if (Blanks.contains(Text[0]))
       SpaceOffset = SplitPoint;
     if (Text[0] == '/')
       SlashOffset = SplitPoint;

[HazardyKnusperkeks]

While it is less code, I find a bit harder to understand and the code gen is far worse: https://gcc.godbolt.org/z/KzG4YnTh3

[owenca]

While it is less code, I find a bit harder to understand and the code gen is far worse: https://gcc.godbolt.org/z/KzG4YnTh3

IsBlank is misleading because of std::isblank whereas Blanks.contains is not, so the latter has better readability for me. As to the generated code, the latter is worse in runtime (IMO negligible in practice), but the code size is about the same with -O2 and far better with -Os (again, negligible in practice). See this modified example, which adds a call to the local function IsBlank to make the comparison fairer.

More importantly, we wouldn't need to update IsBlank if Blanks changes. This happened at least once (580da27) before.

[HazardyKnusperkeks]

While it is less code, I find a bit harder to understand and the code gen is far worse: https://gcc.godbolt.org/z/KzG4YnTh3

IsBlank is misleading because of std::isblank whereas Blanks.contains is not, so the latter has better readability for me. As to the generated code, the latter is worse in runtime (IMO negligible in practice), but the code size is about the same with -O2 and far better with -Os (again, negligible in practice). See this modified example, which adds a call to the local function IsBlank to make the comparison fairer.

More importantly, we wouldn't need to update IsBlank if Blanks changes. This happened at least once (580da27) before.

To make it fair you have to add the static to IsBlank and then it gets inlined again.

It's true, that Blanks and IsBlank have to be maintained simultaneously, but I don't think there will be new blank characters.

And for the readability we obviously disagree.

[owenca]

Had we used StringRef::contains initially, would you still insist that it be replaced with a "more performant" local function? Please note that the code is not on a performance critical path, and the difference in time it takes to run the relevant unit tests for 100 times is within a few milliseconds on my system. For example:

time for i in {1..100}; do FormatTests --gtest_filter=FormatTestComments\.ReflowsComments* > /dev/null; done

Before:

real	0m1.244s
user	0m1.098s
sys	0m0.123s

After:

real	0m1.246s
user	0m1.102s
sys	0m0.121s

Runtime aside, it's bad for maintainability to hardcode the characters in Blanks in a switch statement when there's library function readily available.