[clang-format] Support clang-format on/off line comments as prefixes

[owenpan]

Summary

Closes https://github.com/llvm/llvm-project/issues/60264.

[rymiel]

clang/lib/Format/Format.cpp

3901

Should the space be required? What about // clang-format off: reasoning or similar?

Also, should this be documented?

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

3901

Should the space be required? What about // clang-format off: reasoning or similar?

Also, should this be documented?

+1

[alexolog]

clang/lib/Format/Format.cpp

3893

Here's my attempt at something flexible:
Disclaimer: this is my first time looking at the LLVM codebase, so don't expect production quality.

////////////////////////////////////////////////////////////////////////////////////////////////////

// Implementation detail, hide in a namespace and/or take out of the header
bool isClangFormatMarked(StringRef Comment, StringRef Mark) {
  // Quick heuristics: minimum length and starts with a slash (comment)
  // Shortest tag: "//clang-format on", 17 characters
  static constexpr StringLiteral clangFormatStr("clang-format ");
  if (Comment.size() < clangFormatStr.size() + 4 || Comment[0] != '/')
    return false;

  // check if it's a comment starting with "//" or "/*"
  bool CloseNeeded = false;
  if (Comment[1] == '*')
    CloseNeeded = true;
  else if (Comment[1] != '/')
    return false;

  // Remove the comment start and all following whitespace
  Comment = Comment.substr(2).ltrim();

  // Check for the actual command, a piece at a time
  if (!Comment.consume_front(clangFormatStr) || !Comment.consume_front(Mark))
    return false;

  // Are we there yet?
  if (!CloseNeeded && Comment.empty() ||
      CloseNeeded && Comment.starts_with("*/"))
    return true;

  // For a trailer, restrict the next character
  // (currently spaces and tabs, but can include a colon, etc.)
  static constexpr StringLiteral Separator(" \t");
  if (!Separator.contains(Comment[0]))
    return false;
  
  // Verify comment is properly terminated
  if (!CloseNeeded || Comment.contains("*/"))
    return true;

  return false; // Everything else
}

////////////////////////////////////////////////////////////////////////////////////////////////////

bool isClangFormatOn(StringRef Comment) {
  return isClangFormatMarked(Comment, "on");
}

bool isClangFormatOff(StringRef Comment) {
  return isClangFormatMarked(Comment, "off");
}

////////////////////////////////////////////////////////////////////////////////////////////////////

  EXPECT_TRUE(isClangFormatOn("//clang-format on"));
  EXPECT_TRUE(isClangFormatOn("// clang-format on"));
  EXPECT_TRUE(isClangFormatOn("//clang-format on "));
  EXPECT_TRUE(isClangFormatOn("//clang-format on and off"));
  EXPECT_TRUE(isClangFormatOn("/*clang-format on*/"));
  EXPECT_TRUE(isClangFormatOn("/* clang-format on*/"));
  EXPECT_TRUE(isClangFormatOn("/*clang-format on */"));
  EXPECT_TRUE(isClangFormatOn("/*clang-format on*/int i{0};"));

  EXPECT_FALSE(isClangFormatOn("//clang-format  on"));
  EXPECT_FALSE(isClangFormatOn("//clang-format o"));
  EXPECT_FALSE(isClangFormatOn("//     clang-format o"));
  EXPECT_FALSE(isClangFormatOn("//clang-format ontario"));
  EXPECT_FALSE(isClangFormatOn("//clang-format off"));
  EXPECT_FALSE(isClangFormatOn("/*clang-format onwards*/"));

////////////////////////////////////////////////////////////////////////////////////////////////////

[alexolog]

clang/lib/Format/Format.cpp

3893

Sorry about the "done". My misunderstanding

[owenpan]

clang/lib/Format/Format.cpp

3893

Here's my attempt at something flexible:
Disclaimer: this is my first time looking at the LLVM codebase, so don't expect production quality.

Thanks! If we didn't have to worry about regressions, we might want to do something like what you suggested above.

3901

Should the space be required? What about // clang-format off: reasoning or similar?

On second thought, we should make it more restrictive to avoid regressions. How about requiring a colon, i.e. // clang-format off: (but not // clang-format off :)?

Also, should this be documented?

Yep.

[owenpan]

Only allows : as the first character after // clang-format on or // clang-format off.

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

3901

Should the space be required? What about // clang-format off: reasoning or similar?

On second thought, we should make it more restrictive to avoid regressions. How about requiring a colon, i.e. // clang-format off: (but not // clang-format off :)?

That's fine by me. But why not also /**/?

[owenpan]

Improves isClangFormatOnOff() a little bit.

[owenpan]

clang/lib/Format/Format.cpp

3901

But why not also /**/?

If it weren't for the fact that the code which checks for clang-format on/off exists in several places, I wouldn't want this feature added. IMO there's no need to allow /* clang-format off: */ if we got // clang-format: and being more restrictive results in a lower risk of regression.

[alexolog]

clang/lib/Format/Format.cpp

3893

Isn't it what extensive test coverage is for?

[MyDeveloperDay]

LGTM

[owenpan]

clang/lib/Format/Format.cpp

3893

I meant the regression of behavior.

clang/docs/ClangFormatStyleOptions.rst

 
 
 Disabling Formatting on a Piece of Code
 =======================================
 
 Clang-format understands also special comments that switch formatting in a
 delimited range. The code between a comment ``// clang-format off`` or
 ``/* clang-format off */`` up to a comment ``// clang-format on`` or
-``/* clang-format on */`` will not be formatted. The comments themselves
-will be formatted (aligned) normally.
+``/* clang-format on */`` will not be formatted. The comments themselves will be
+formatted (aligned) normally. Also, a colon (``:``) and additional text may
+follow ``// clang-format off`` or `` clang-format on`` to explain why
+clang-format is turned off or back on.
 
 .. code-block:: c++
 
   int formatted_code;
   // clang-format off
       void    unformatted_code  ;
   // clang-format on
   void formatted_code_again;

clang/include/clang/Format/Format.h

case FormatStyle::LK_TextProto:
     return "TextProto";
   case FormatStyle::LK_Verilog:
     return "Verilog";
   default:
     return "Unknown";
   }
 }
 
+bool isClangFormatOn(StringRef Comment);
+bool isClangFormatOff(StringRef Comment);
+
 } // end namespace format
 } // end namespace clang
 
 namespace std {
 template <>
 struct is_error_code_enum<clang::format::ParseError> : std::true_type {};
 } // namespace std
 
 #endif // LLVM_CLANG_FORMAT_FORMAT_H

clang/lib/Format/Format.cpp

if (RawStringRegex.match(Trimmed, &RawStringMatches)) {
       std::string CharSequence = RawStringMatches[1].str();
       RawStringTermination = ")" + CharSequence + "\"";
       FormattingOff = true;
     }
 
     if (Trimmed.contains(RawStringTermination))
       FormattingOff = false;
 
-    if (Trimmed == "// clang-format off" ||
-        Trimmed == "/* clang-format off */") {
+    if (isClangFormatOff(Trimmed))
       FormattingOff = true;
-    } else if (Trimmed == "// clang-format on" ||
-               Trimmed == "/* clang-format on */") {
+    else if (isClangFormatOn(Trimmed))
       FormattingOff = false;
-    }
 
     const bool EmptyLineSkipped =
         Trimmed.empty() &&
         (Style.IncludeStyle.IncludeBlocks == tooling::IncludeStyle::IBS_Merge ||
          Style.IncludeStyle.IncludeBlocks ==
              tooling::IncludeStyle::IBS_Regroup);
 
     bool MergeWithNextLine = Trimmed.endswith("\\");
tooling::Replacements sortJavaImports(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 (isClangFormatOff(Trimmed))
       FormattingOff = true;
-    else if (Trimmed == "// clang-format on")
+    else if (isClangFormatOn(Trimmed))
       FormattingOff = false;
 
     if (ImportRegex.match(Line, &Matches)) {
       if (FormattingOff) {
         // If at least one import line has formatting turned off, turn off
         // formatting entirely.
         return Replaces;
       }
if (!ChildFormatTextToApply.empty()) {
     LLVM_DEBUG(llvm::dbgs()
                << "Applying child configurations on fallback style\n");
     applyChildFormatTexts(&FallbackStyle);
   }
 
   return FallbackStyle;
 }
 
+static bool isClangFormatOnOff(StringRef Comment, bool On) {
+  if (Comment == (On ? "/* clang-format on */" : "/* clang-format off */"))
+    return true;
+
+  static const char ClangFormatOn[] = "// clang-format on";
+  static const char ClangFormatOff[] = "// clang-format off";
+  const unsigned Size = (On ? sizeof ClangFormatOn : sizeof ClangFormatOff) - 1;
+
+  return Comment.startswith(On ? ClangFormatOn : ClangFormatOff) &&
+         (Comment.size() == Size || Comment[Size] == ':');
+}
+
+bool isClangFormatOn(StringRef Comment) {
+  return isClangFormatOnOff(Comment, /*On=*/true);
+}
+
+bool isClangFormatOff(StringRef Comment) {
+  return isClangFormatOnOff(Comment, /*On=*/false);
+}
+
 } // namespace format
 } // namespace clang

clang/lib/Format/FormatTokenLexer.cpp

void FormatTokenLexer::readRawToken(FormatToken &Tok) {
   }
 
   if ((Style.isJavaScript() || Style.Language == FormatStyle::LK_Proto ||
        Style.Language == FormatStyle::LK_TextProto) &&
       Tok.is(tok::char_constant)) {
     Tok.Tok.setKind(tok::string_literal);
   }
 
-  if (Tok.is(tok::comment) && (Tok.TokenText == "// clang-format on" ||
-                               Tok.TokenText == "/* clang-format on */")) {
+  if (Tok.is(tok::comment) && isClangFormatOn(Tok.TokenText))
     FormattingDisabled = false;
-  }
 
   Tok.Finalized = FormattingDisabled;
 
-  if (Tok.is(tok::comment) && (Tok.TokenText == "// clang-format off" ||
-                               Tok.TokenText == "/* clang-format off */")) {
+  if (Tok.is(tok::comment) && isClangFormatOff(Tok.TokenText))
     FormattingDisabled = true;
-  }
-}
 
 void FormatTokenLexer::resetLexer(unsigned Offset) {
   StringRef Buffer = SourceMgr.getBufferData(ID);
   LangOpts = getFormattingLangOpts(Style);
   Lex.reset(new Lexer(SourceMgr.getLocForStartOfFile(ID), LangOpts,
                       Buffer.begin(), Buffer.begin() + Offset, Buffer.end()));
   Lex->SetKeepWhitespaceMode(true);
   TrailingWhitespace = 0;
 }
 
 } // namespace format
 } // namespace clang

clang/lib/Format/IntegerLiteralSeparatorFixer.cpp

IntegerLiteralSeparatorFixer::process(const Environment &Env,
 
   for (bool Skip = false; !Lex.LexFromRawLexer(Tok);) {
     auto Length = Tok.getLength();
     if (Length < 2)
       continue;
     auto Location = Tok.getLocation();
     auto Text = StringRef(SourceMgr.getCharacterData(Location), Length);
     if (Tok.is(tok::comment)) {
-      if (Text == "// clang-format off" || Text == "/* clang-format off */")
+      if (isClangFormatOff(Text))
         Skip = true;
-      else if (Text == "// clang-format on" || Text == "/* clang-format on */")
+      else if (isClangFormatOn(Text))
         Skip = false;
       continue;
     }
     if (Skip || Tok.isNot(tok::numeric_constant) || Text[0] == '.' ||
         !AffectedRangeMgr.affectsCharSourceRange(
             CharSourceRange::getCharRange(Location, Tok.getEndLoc()))) {
       continue;
     }

clang/lib/Format/SortJavaScriptImports.cpp

analyze(TokenAnnotator &Annotator,
     // FIXME: A better long term fix is to re-calculate Ranges after sorting.
     unsigned PreviousSize = PreviousText.size();
     while (ReferencesText.size() < PreviousSize)
       ReferencesText += " ";
 
     // Separate references from the main code body of the file.
     if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2 &&
         !(FirstNonImportLine->First->is(tok::comment) &&
-          FirstNonImportLine->First->TokenText.trim() ==
-              "// clang-format on")) {
+          isClangFormatOn(FirstNonImportLine->First->TokenText.trim()))) {
       ReferencesText += "\n";
     }
 
     LLVM_DEBUG(llvm::dbgs() << "Replacing imports:\n"
                             << PreviousText << "\nwith:\n"
                             << ReferencesText << "\n");
     auto Err = Result.add(tooling::Replacement(
         Env.getSourceManager(), CharSourceRange::getCharRange(InsertionPoint),
parseModuleReferences(const AdditionalKeywords &Keywords,
     for (auto *Line : AnnotatedLines) {
       assert(Line->First);
       Current = Line->First;
       LineEnd = Line->Last;
       // clang-format comments toggle formatting on/off.
       // This is tracked in FormattingOff here and on JsModuleReference.
       while (Current && Current->is(tok::comment)) {
         StringRef CommentText = Current->TokenText.trim();
-        if (CommentText == "// clang-format off") {
+        if (isClangFormatOff(CommentText)) {
           FormattingOff = true;
-        } else if (CommentText == "// clang-format on") {
+        } else if (isClangFormatOn(CommentText)) {
           FormattingOff = false;
           // Special case: consider a trailing "clang-format on" line to be part
           // of the module reference, so that it gets moved around together with
           // it (as opposed to the next module reference, which might get sorted
           // around).
           if (!References.empty()) {
             References.back().Range.setEnd(Current->Tok.getEndLoc());
             Start = Current->Tok.getEndLoc().getLocWithOffset(1);

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("// clang-format off\n"
             format("// clang-format off\n"
                    "// long long long long long long line\n"
                    "/* clang-format on */\n"
                    "/* long long long long long long line */\n"
                    "int i;\n"
                    "/* clang-format off */\n"
                    "/* long long long long long long line */\n",
                    getLLVMStyleWithColumns(20)));
+
+  verifyFormat("int *i;\n"
+               "// clang-format off:\n"
+               "int* j;\n"
+               "// clang-format on: 1\n"
+               "int *k;",
+               "int* i;\n"
+               "// clang-format off:\n"
+               "int* j;\n"
+               "// clang-format on: 1\n"
+               "int* k;");
+
+  verifyFormat("int *i;\n"
+               "// clang-format off:0\n"
+               "int* j;\n"
+               "// clang-format only\n"
+               "int* k;",
+               "int* i;\n"
+               "// clang-format off:0\n"
+               "int* j;\n"
+               "// clang-format only\n"
+               "int* k;");
 }
 
 TEST_F(FormatTest, DoNotCrashOnInvalidInput) {
   format("? ) =");
   verifyNoCrash("#define a\\\n /**/}");
 }
 
 TEST_F(FormatTest, FormatsTableGenCode) {