clang-format: Add option to remove semicolon at end of namespace

[Typz]

Summary

This option allows cleaning up namespace declaration, by removing the
extra semicolon after namespace closing brace.

[krasimir]

I think that this is more of a linter check and as such doesn't really belong to clang-format.
@djasper: what do you think about this?

[Typz]

Is this not the same reasoning as the whole NamespaceEndCommentsFixer?

[djasper]

Yes, this definitely does not belong in the NamespaceEndCommentsFixer. It has nothing to do with comments.

And I am also very skeptical about several things:

• Why start here? There are many places where semicolons could be cleaned up.
• If we add more of such cleanup functionality, I think we should pull them out of the general configuration. This isn't really part of a "style".
• We do have some somewhat related functionality to cleanup in the Cleaner class in Format.cpp.

Before going forward with this, I'd like to understand what the long-term plan is.

[Typz]

I stumbled on the issue when working on the CompactNamespaces option, where the extra semicolon prevents merging the closing braces.
There was an easy fix, which guaranteed that the closing braces would be properly merged, so I went for it, just adding an option to keep things generic.

So there wasn't really a "long term" plan (though since I made the fix, I have though that it would be nice to remove other cases of extra semicolon, e.g. after function body).

I did not know of this 'Cleaner' class (I did not actually try to find it either, thruth be told), but it would seem more appropriate indeed. The only thing is I could not find where this is called, and esp. confirm if it happens before the NamespaceEndCommentsFixer.

Back to the point, I can see a few options here:

• Merge this patch into the CompactNamespaces patch, but remove the option and drop the semicolon only when CompactNamespaces mode is enabled
• Rework CompactNamespaces to merge namespace closing brace with extra semicolon (not a big fan, it would look very strange: };})
• Implement this in a more generic fashion, in the Cleaner, with no style option (e.g. always on). Possibly the better option, but may be a more significant significant undertaking

[djasper]

I think we should just not do this for now. This addresses a very infrequent case that's easy enough to fix manually. As such, it's not worth the added complexity of clang-format and potential failures it might generate. Changing non-whitespace/non-comment code is always dangerous.

What's the current behavior of the compact namespaces in the presence of semicolons?

[Typz]

ATM, in the presence of semicolons, the closing braces will simply not be merged : i will check if I can handle this case easily (in the CompactNamespaces patch), and I'll abandon this one for now.

[anatol.pomozov]

Would it be possible to add a Fixer that removes unneeded semicolon after C function?

include/clang/Format/Format.h

struct FormatStyle {
   /// \code
   ///   true:                                   false:
   ///   myFunction(foo,                 vs.     myFunction(foo, bar, plop);
   ///              bar,
   ///              plop);
   /// \endcode
   bool AllowAllParametersOfDeclarationOnNextLine;
 
+  /// \brief If ``true``, clang-format removes semicolon at the end of namespace.
+  /// \code
+  ///    true:                                  false:
+  ///    namespace a {                  vs.     namespace a {
+  ///    }                                      };
+  /// \endcode
+  bool AllowSemicolonAfterNamespace;
+
   /// \brief Allows contracting simple braced statements to a single line.
   ///
   /// E.g., this allows ``if (a) { return; }`` to be put on a single line.
   bool AllowShortBlocksOnASingleLine;
 
   /// \brief If ``true``, short case labels will be contracted to a single line.
   /// \code
   ///   true:                                   false:
return AccessModifierOffset == R.AccessModifierOffset &&
            AlignAfterOpenBracket == R.AlignAfterOpenBracket &&
            AlignConsecutiveAssignments == R.AlignConsecutiveAssignments &&
            AlignConsecutiveDeclarations == R.AlignConsecutiveDeclarations &&
            AlignEscapedNewlinesLeft == R.AlignEscapedNewlinesLeft &&
            AlignOperands == R.AlignOperands &&
            AlignTrailingComments == R.AlignTrailingComments &&
            AllowAllParametersOfDeclarationOnNextLine ==
                R.AllowAllParametersOfDeclarationOnNextLine &&
+           AllowSemicolonAfterNamespace == R.AllowSemicolonAfterNamespace &&
            AllowShortBlocksOnASingleLine == R.AllowShortBlocksOnASingleLine &&
            AllowShortCaseLabelsOnASingleLine ==
                R.AllowShortCaseLabelsOnASingleLine &&
            AllowShortFunctionsOnASingleLine ==
                R.AllowShortFunctionsOnASingleLine &&
            AllowShortIfStatementsOnASingleLine ==
                R.AllowShortIfStatementsOnASingleLine &&
            AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&

lib/Format/Format.cpp

IO.mapOptional("AlignConsecutiveAssignments",
                    Style.AlignConsecutiveAssignments);
     IO.mapOptional("AlignConsecutiveDeclarations",
                    Style.AlignConsecutiveDeclarations);
     IO.mapOptional("AlignEscapedNewlinesLeft", Style.AlignEscapedNewlinesLeft);
     IO.mapOptional("AlignOperands", Style.AlignOperands);
     IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);
     IO.mapOptional("AllowAllParametersOfDeclarationOnNextLine",
                    Style.AllowAllParametersOfDeclarationOnNextLine);
+    IO.mapOptional("AllowSemicolonAfterNamespace", Style.AllowSemicolonAfterNamespace);
     IO.mapOptional("AllowShortBlocksOnASingleLine",
                    Style.AllowShortBlocksOnASingleLine);
     IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
                    Style.AllowShortCaseLabelsOnASingleLine);
     IO.mapOptional("AllowShortFunctionsOnASingleLine",
                    Style.AllowShortFunctionsOnASingleLine);
     IO.mapOptional("AllowShortIfStatementsOnASingleLine",
                    Style.AllowShortIfStatementsOnASingleLine);
FormatStyle getLLVMStyle() {
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlinesLeft = false;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignOperands = true;
   LLVMStyle.AlignTrailingComments = true;
   LLVMStyle.AlignConsecutiveAssignments = false;
   LLVMStyle.AlignConsecutiveDeclarations = false;
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
+  LLVMStyle.AllowSemicolonAfterNamespace = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;

lib/Format/NamespaceEndCommentsFixer.cpp

void updateEndComment(const FormatToken *RBraceTok, StringRef EndCommentText,
   auto Range = CharSourceRange::getCharRange(Comment->getStartOfNonWhitespace(),
                                              Comment->Tok.getEndLoc());
   auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, EndCommentText));
   if (Err) {
     llvm::errs() << "Error while updating namespace end comment: "
                  << llvm::toString(std::move(Err)) << "\n";
   }
 }
+
+void removeTrailingSemicolon(const FormatToken *SemiColonTok,
+                             const SourceManager &SourceMgr,
+                             tooling::Replacements *Fixes) {
+  assert(SemiColonTok->is(tok::semi));
+  auto Range = CharSourceRange::getCharRange(SemiColonTok->Tok.getLocation(),
+                                             SemiColonTok->Tok.getEndLoc());
+  auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, StringRef()));
+  if (Err) {
+    llvm::errs() << "Error while removing trailing semicolon at end of namespace: "
+                 << llvm::toString(std::move(Err)) << "\n";
+  }
+}
 } // namespace
 
 NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
                                                      const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 tooling::Replacements NamespaceEndCommentsFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
     if (RBraceTok->Finalized)
       continue;
     RBraceTok->Finalized = true;
     const FormatToken *EndCommentPrevTok = RBraceTok;
     // Namespaces often end with '};'. In that case, attach namespace end
     // comments to the semicolon tokens.
     if (RBraceTok->Next && RBraceTok->Next->is(tok::semi)) {
       EndCommentPrevTok = RBraceTok->Next;
+      if (!Style.AllowSemicolonAfterNamespace)
+        removeTrailingSemicolon(RBraceTok->Next, SourceMgr, &Fixes);
     }
     // The next token in the token stream after the place where the end comment
     // token must be. This is either the next token on the current line or the
     // first token on the next line.
     const FormatToken *EndCommentNextTok = EndCommentPrevTok->Next;
     if (EndCommentNextTok && EndCommentNextTok->is(tok::comment))
       EndCommentNextTok = EndCommentNextTok->Next;
     if (!EndCommentNextTok && I + 1 < E)

unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE_BOOL(AlignEscapedNewlinesLeft);
   CHECK_PARSE_BOOL(AlignOperands);
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
   CHECK_PARSE_BOOL(AlignConsecutiveDeclarations);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
+  CHECK_PARSE_BOOL(AllowSemicolonAfterNamespace);
   CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);

unittests/Format/NamespaceEndCommentsFixerTest.cpp

EXPECT_EQ("namespace A {\n"
             "// unrelated",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int i;\n"
                                     "  int j;\n"
                                     "};\n"
                                     "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, RemoveSemicolon) {
+  FormatStyle LLVMStyleWithoutSemicolon = getLLVMStyle();
+  LLVMStyleWithoutSemicolon.AllowSemicolonAfterNamespace = false;
+
+  EXPECT_EQ("namespace {\n"
+            "  int i;\n"
+            "  int j;\n"
+            "}// namespace",
+            fixNamespaceEndComments("namespace {\n"
+                                    "  int i;\n"
+                                    "  int j;\n"
+                                    "};",
+                                    LLVMStyleWithoutSemicolon));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "  int j;\n"
+            "}// namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "  int j;\n"
+                                    "};",
+                                    LLVMStyleWithoutSemicolon));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "  int j;\n"
+            "}// namespace A\n"
+            "// unrelated",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "  int j;\n"
+                                    "};\n"
+                                    "// unrelated",
+                                    LLVMStyleWithoutSemicolon));
+  // case without semicolon is not affected
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "  int j;\n"
+            "}// namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "  int j;\n"
+                                    "}",
+                                    LLVMStyleWithoutSemicolon));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
             "  int i;\n"
             "  int j;\n"
             "}// namespace A\n"
             " int k;",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int i;\n"