[clang-format] [PR46130] When editing a file with unbalance {} the namespace comment fixer can incorrectly comment the wrong closing brace

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=46130 from Twitter https://twitter.com/ikautak/status/1265998988232159232

I have seen this myself many times.. if you have format on save and you work in an editor where you are constantly saving (:w muscle memory)

If you are in the middle of editing and somehow you've missed a { or } in your code, somewhere, often way below where you are at the bottom of your file the namespace comment fixer will have put the namespace on the previous closing brace.

This leads to you having to fix up the bottom of the file.

This revision prevents that happening by performing an initial pass of the tokens and simply counting the number of { and } and ensuring they balance.

If they don't balance we don't do any namespace fixing as it will likely be unstable and incorrect.

[curdeius]

LGTM.
It may be silly, but the fact that this code runs over all the tokens makes me think: do we have any performance-related non-regression tests for clang-format?

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

1093

Nit: You may add PR46130 in the test name *if* this is what is done usually (I see it in the test above).

[MyDeveloperDay]

We don't really have any performance tests, but the following shows some analysis, all builds are release build:

• clang-format-ns is built with this change (compiled with VS2017)
• clang-format is without (compiled with VS2017)
• c:/Program\ Files/LLVM/bin/clang-format is the last Feb 2020 snapshot build version of clang-format (probably compiled with clang-cl)
• build_release_cl/clang-format with this change (compiled with clang-cl from Fab2020 snapshot) _ build_2019/clang-format with this change (compiled with VS2019)

The test is to run clang-format over the largest file in LLVM

./libcxxabi/test/test_demangle.pass.cpp is 5.2M in size = 30,000 lines, 29799 opening and closing braces

For a file this size I think I'd expect IO to dominate (I'm running this test on an Intel i7-800 CPU @3,.20GHz with an SSD, Running cygwin in Windows 10)

I've run these tests multiple times and they are often the same or similar timings (I'm running the base clang-format as a benchmark twice because the first run is often slower

time /clang/build_release/bin/clang-format ../libcxxabi/test/test_demangle.pass.cpp > speed1 (Baseline to get test_demangle.pass.cpp into system cache)

real    0m3.957s
user    0m0.000s
sys     0m0.000s

time /clang/build_release/bin/clang-format-ns ../libcxxabi/test/test_demangle.pass.cpp > speed1

real    0m3.994s
user    0m0.000s
sys     0m0.000s
time /clang/build_release/bin/clang-format ../libcxxabi/test/test_demangle.pass.cpp > speed1

real    0m3.990s
user    0m0.000s
sys     0m0.000s

time  c:/Program\ Files/LLVM/bin/clang-format ../libcxxabi/test/test_demangle.pass.cpp > speed1

real    0m3.793s
user    0m0.000s
sys     0m0.000s

time /clang/build_release_cl/bin/clang-format ../libcxxabi/test/test_demangle.pass.cpp > speed1

real    0m3.442s
user    0m0.000s
sys     0m0.000s

time /clang/build_2019/bin/clang-format ../libcxxabi/test/test_demangle.pass.cpp > speed1

real    0m3.820s
user    0m0.000s
sys     0m0.000s

The speed difference of this change of clang-format is fairly negligible, varying by about +/-20ms one way or the other (often swinging the other between runs or depending on which goes first)

I feel this change is relatively low impact, (perhaps the loop could be moved into some earlier equivalent loop but for clarity, I think it's ok here for now.)

I'm not 100% sure how we could reliably test performance generally in clang-format, but I think it would be good to have something.

The key takeaway is likely that clang-cl is quicker! and it obviously more about which compiler you choose.

[curdeius]

OK, great. Thanks for all the information.

[MaskRay]

arc adds many unneeded tags from Phabricator. You can drop Reviewers: Subscribers: Tags: and the text Summary: with the following script:

arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people (https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html). You should keep the tag. (I started to use --date=now because some people find author date != committer date annoying. The committer date is usually what people care.))

clang/lib/Format/NamespaceEndCommentsFixer.cpp

NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
     : TokenAnalyzer(Env, Style) {}
 
 std::pair<tooling::Replacements, unsigned> NamespaceEndCommentsFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {
   const SourceManager &SourceMgr = Env.getSourceManager();
   AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
   tooling::Replacements Fixes;
+
+  // Spin through the lines and ensure we have balanced braces.
+  int Braces = 0;
+  for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
+    FormatToken *Tok = AnnotatedLines[I]->First;
+    while (Tok) {
+      Braces += Tok->is(tok::l_brace) ? 1 : Tok->is(tok::r_brace) ? -1 : 0;
+      Tok = Tok->Next;
+    }
+  }
+  // Don't attempt to comment unbalanced braces or this can
+  // lead to comments being placed on the closing brace which isn't
+  // the matching brace of the namespace. (occurs during incomplete editing).
+  if (Braces != 0) {
+    return {Fixes, 0};
+  }
+
   std::string AllNamespaceNames = "";
   size_t StartLineIndex = SIZE_MAX;
   StringRef NamespaceTokenText;
   unsigned int CompactedNamespacesCount = 0;
   for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
     const AnnotatedLine *EndLine = AnnotatedLines[I];
     const FormatToken *NamespaceTok =
         getNamespaceToken(EndLine, AnnotatedLines);

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

EXPECT_EQ("template <int> struct a {};\n"
             "}\n",
             fixNamespaceEndComments("template <int> struct a {};\n"
                                     "struct a<bool{}> b() {\n"
                                     "}\n"
                                     "#define c inline\n"
                                     "void d() {\n"
                                     "}\n"));
 }
+
+TEST_F(NamespaceEndCommentsFixerTest, IgnoreUnbalanced) {
+  EXPECT_EQ("namespace A {\n"
+            "class Foo {\n"
+            "}\n"
+            "}// namespace A\n",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "class Foo {\n"
+                                    "}\n"
+                                    "}\n"));
+  EXPECT_EQ("namespace A {\n"
+            "class Foo {\n"
+            "}\n",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "class Foo {\n"
+                                    "}\n"));
+
+  EXPECT_EQ("namespace A {\n"
+            "class Foo {\n"
+            "}\n"
+            "}\n"
+            "}\n",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "class Foo {\n"
+                                    "}\n"
+                                    "}\n"
+                                    "}\n"));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang