[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Anatol Pomozov via Phabricator via cfe-commits]

anatol.pomozov added a comment.

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


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Francois Ferrand via Phabricator via cfe-commits]

Typz abandoned this revision.
Typz added a comment.

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.


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

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?


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

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


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

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.


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Is this not the same reasoning as the whole NamespaceEndCommentsFixer?


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

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?


https://reviews.llvm.org/D33314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Herald added a subscriber: klimek.

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


https://reviews.llvm.org/D33314

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -214,6 +214,51 @@
 "// 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"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8670,6 +8670,7 @@
   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);
Index: lib/Format/NamespaceEndCommentsFixer.cpp
===
--- lib/Format/NamespaceEndCommentsFixer.cpp
+++ lib/Format/NamespaceEndCommentsFixer.cpp
@@ -107,6 +107,19 @@
  << llvm::toString(std::move(Err)) << "\n";
   }
 }
+
+void removeTrailingSemicolon(const FormatToken *SemiColonTok,
+ const SourceManager ,
+ 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 ,
@@ -144,6 +157,8 @@
 // comments to the semicolon tokens.
 if (RBraceTok->Next && RBraceTok->Next->is(tok::semi)) {
   EndCommentPrevTok = RBraceTok->Next;
+  if (!Style.AllowSemicolonAfterNamespace)
+removeTrailingSemicolon(RBraceTok->Next, SourceMgr, );
 }
 // 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
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -252,6 +252,7 @@
 IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments);