[clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

[SimplyDanny]

Summary

This is a fix for #53737. In addition to commas, newline characters are considered as separators of checks.

[njames93]

.clang-tidy

1–10 ↗

(On Diff #410088)

This change doesn't belong in this patch.

clang-tools-extra/clang-tidy/GlobList.cpp

30–31

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

100

This seems like a shortcoming in llvms YAML parser. Isn't the fold character > supposed to replace newlines with spaces and strip trailing ws

"-*,misc-* llvm-* -clang-* google-*"

Using the pipe | instead should yield the output you are currently expecting,

[SimplyDanny]

Update according to comments in review

[SimplyDanny]

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

100

That's true. > should actually not work without a comma after each check.

[SimplyDanny]

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

100

YAMLParser.h mentions that "Multi-line literal folding" is not yet implemented.

[njames93]

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

100

So it is, D102590 has been up addressing this very issue (Though its rather stale ATM).
In any case for now we shouldn't depend on the broken behaviour.

[HassanElDesouky]

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

100

Thanks for motivating me to get D102590 to the finish line. I think it's ready now.

[SimplyDanny]

What else can be done to get this change approved, @njames93?

[aaron.ballman]

LGTM, but please wait a day or two before landing in case @njames93 has further concerns.

[SimplyDanny]

In D120187#3375610, @aaron.ballman wrote:

LGTM, but please wait a day or two before landing in case @njames93 has further concerns.

Would you merge the changes, @aaron.ballman? Seems like there are no objections from others. Please use the commit details mentioned here.

[aaron.ballman]

In D120187#3382973, @SimplyDanny wrote:

In D120187#3375610, @aaron.ballman wrote:

LGTM, but please wait a day or two before landing in case @njames93 has further concerns.

Would you merge the changes, @aaron.ballman? Seems like there are no objections from others. Please use the commit details mentioned here.

I've commit on your behalf in 2b21fc5520b39fba555f4e5f2480a5651056be84, thank you for the fix!

clang-tools-extra/clang-tidy/GlobList.cpp

if (GlobList.startswith("-")) {
     return true;
   }
   return false;
 }
 
 // Converts first glob from the comma-separated list of globs to Regex and
 // removes it and the trailing comma from the GlobList.
 static llvm::Regex consumeGlob(StringRef &GlobList) {
-  StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find(','));
+  StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find_first_of(",\n"));
   StringRef Glob = UntrimmedGlob.trim();
   GlobList = GlobList.substr(UntrimmedGlob.size() + 1);
   SmallString<128> RegexText("^");
   StringRef MetaChars("()^$|*+?.[]\\{}");
   for (char C : Glob) {
     if (C == '*')
       RegexText.push_back('.');
     else if (MetaChars.contains(C))
       RegexText.push_back('\\');
     RegexText.push_back(C);
   }
   RegexText.push_back('$');
   return llvm::Regex(RegexText);
 }
 
 GlobList::GlobList(StringRef Globs, bool KeepNegativeGlobs /* =true */) {
-  Items.reserve(Globs.count(',') + 1);
+  Items.reserve(Globs.count(',') + Globs.count('\n') + 1);
   do {
     GlobListItem Item;
     Item.IsPositive = !consumeNegativeIndicator(Globs);
     Item.Regex = consumeGlob(Globs);
     if (Item.IsPositive || KeepNegativeGlobs)
       Items.push_back(std::move(Item));
   } while (!Globs.empty());
 }

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

llvm::ErrorOr<ClangTidyOptions> Options =
                                                "User: some.user",
                                                "Options"));
   EXPECT_TRUE(!!Options);
   EXPECT_EQ("-*,misc-*", *Options->Checks);
   EXPECT_EQ(".*", *Options->HeaderFilterRegex);
   EXPECT_EQ("some.user", *Options->User);
 }
 
+TEST(ParseConfiguration, ChecksSeparatedByNewlines) {
+  auto MemoryBuffer = llvm::MemoryBufferRef("Checks: |\n"
+                                            "  -*,misc-*\n"
+                                            "  llvm-*\n"
+                                            "  -clang-*,\n"
+                                            "  google-*",
+                                            "Options");
+
+  auto Options = parseConfiguration(MemoryBuffer);
+
+  EXPECT_TRUE(!!Options);
+  EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks);
+}
+
 TEST(ParseConfiguration, MergeConfigurations) {
   llvm::ErrorOr<ClangTidyOptions> Options1 =
       parseConfiguration(llvm::MemoryBufferRef(R"(
       Checks: "check1,check2"
       HeaderFilterRegex: "filter1"
       AnalyzeTemporaryDtors: true
       User: user1
       ExtraArgs: ['arg1', 'arg2']

clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp

TYPED_TEST(GlobListTest, Complex) {
   EXPECT_FALSE(Filter.contains("b.b"));
   EXPECT_TRUE(Filter.contains("a.1.b"));
   EXPECT_FALSE(Filter.contains("a.1.A.a"));
   EXPECT_FALSE(Filter.contains("qwe"));
   EXPECT_FALSE(Filter.contains("asdfqweasdf"));
   EXPECT_TRUE(Filter.contains("asdfqwEasdf"));
 }
 
+TYPED_TEST(GlobListTest, NewlineCharactersAsSeparators) {
+  TypeParam Filter("a*  \n b,\n-c*,dd");
+
+  EXPECT_FALSE(Filter.contains(""));
+  EXPECT_TRUE(Filter.contains("aaa"));
+  EXPECT_TRUE(Filter.contains("b"));
+  EXPECT_FALSE(Filter.contains("c"));
+  EXPECT_FALSE(Filter.contains("ccc"));
+  EXPECT_FALSE(Filter.contains("d"));
+  EXPECT_TRUE(Filter.contains("dd"));
+  EXPECT_FALSE(Filter.contains("ddd"));
+}
+
 } // namespace tidy
 } // namespace clang