[clang-format] clang-format off/on not respected when using C Style comments

[MyDeveloperDay]

Summary

If the clang-format on/off is in a /* comment */ then the sorting of headers is not ignored

PR40901 - https://bugs.llvm.org/show_bug.cgi?id=40901

[alexfh]

clang/unittests/Format/SortIncludesTest.cpp

132

Add a test with /* clang-format officially supports C++ */ ;)

Seriously speaking, the startswith() condition in the code should be a bit stricter. Maybe just compare with /* clang-format off */ and /* clang-format on */? If there's a motivating use case for just checking the prefix, could you add it to the test?

[MyDeveloperDay]

clang/unittests/Format/SortIncludesTest.cpp

132

You know i thought I was being so clever by not having the space after ..off to avoid

/*clang-format off*/

That'll teach me!

[MyDeveloperDay]

Fix negative test case
support the same /*<space>clang-format off<space>*/ in the sort includes that the TokenLexer supports.

[JonasToth]

clang/lib/Format/Format.cpp

1793

Should we allow

/* clang-format off
    It is just horrible for this piece of code. */

? Multiline-comments could span multiple lines and to deactivates clang-format and give reasons.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1793

initially that is kind of what I was trying to do, but if you look over in TokenLexer where its turned off for styling they check only for a single line

So to be honest if we are going to change it we should change it in both places, but perhaps that is just overkill.

[JonasToth]

clang/lib/Format/Format.cpp

1793

Totally, not necessary in my eyes, nice plus, so dont implement it :D

[alexfh]

LG

clang/lib/Format/Format.cpp

tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
   bool FormattingOff = false;
 
   for (;;) {
     auto Pos = Code.find('\n', SearchFrom);
     StringRef Line =
         Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
 
     StringRef Trimmed = Line.trim();
-    if (Trimmed == "// clang-format off")
+    if (Trimmed == "// clang-format off" ||
+        Trimmed.startswith("/* clang-format off"))
       FormattingOff = true;
-    else if (Trimmed == "// clang-format on")
+    else if (Trimmed == "// clang-format on" ||
+             Trimmed.startswith("/* clang-format on"))
       FormattingOff = false;
 
     const bool EmptyLineSkipped =
         Trimmed.empty() &&
         (Style.IncludeStyle.IncludeBlocks == tooling::IncludeStyle::IBS_Merge ||
          Style.IncludeStyle.IncludeBlocks ==
              tooling::IncludeStyle::IBS_Regroup);
 

clang/unittests/Format/SortIncludesTest.cpp

EXPECT_EQ("#include <a>\n"
                  "#include <c>\n"
                  "// clang-format off\n"
                  "#include <b>\n"
                  "#include <a>\n"
                  "#include <c>\n"
                  "// clang-format on\n"));
 }
 
+TEST_F(SortIncludesTest, SupportClangFormatOffCStyle) {
+  EXPECT_EQ("#include <a>\n"
+            "#include <b>\n"
+            "#include <c>\n"
+            "/* clang-format off */\n"
+            "#include <b>\n"
+            "#include <a>\n"
+            "#include <c>\n"
+            "/* clang-format on */\n",
+            sort("#include <b>\n"
+                 "#include <a>\n"
+                 "#include <c>\n"
+                 "/* clang-format off */\n"
+                 "#include <b>\n"
+                 "#include <a>\n"
+                 "#include <c>\n"
+                 "/* clang-format on */\n"));
+}
+
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
   FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
             "#include \"c.h\"\n"
             "#include \"b.h\"\n",
             sort("#include \"a.h\"\n"
                  "#include \"c.h\"\n"
                  "#include \"b.h\"\n"));