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

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/Format.cpp:3893
 
+bool isClangFormatOn(StringRef Comment) {
+  if (Comment == "/* clang-format on */")
----------------
alexolog wrote:
> alexolog wrote:
> > 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*/"));
> > 
> > ////////////////////////////////////////////////////////////////////////////////////////////////////
> > ```
> Sorry about the "done".  My misunderstanding
> 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.


================
Comment at: clang/lib/Format/Format.cpp:3901
+  return Comment.startswith(Prefix) &&
+         (Comment.size() == Size || isblank(Comment[Size]));
+}
----------------
HazardyKnusperkeks wrote:
> rymiel wrote:
> > Should the space be required? What about `// clang-format off: reasoning` or similar?
> > 
> > Also, should this be documented?
> > Should the space be required? What about `// clang-format off: reasoning` or similar?
> > 
> > Also, should this be documented?
> 
> +1
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142804