[clang-format][PR47290] Add ShortNamespaceLines format option

[kuzkry]

Summary

clang-format documentation states that having enabled
FixNamespaceComments one may expect below code:

c++
namespace a {
foo();
}

to be turned into:

c++
namespace a {
foo();
} // namespace a

In reality, no "// namespace a" was added. The problem was too high
value of kShortNamespaceMaxLines, which is used while deciding whether
a namespace is long enough to be formatted.

As with 9163fe2, clang-format idempotence is preserved.

[kuzkry]

This fixes https://bugs.llvm.org/show_bug.cgi?id=47290.

@Jake
I decided I would at least create this pull request so you can decide whether you want it or not.

[kuzkry]

Adjusted tests IncludeFixerTest.cpp after CI failure.

[kuzkry]

Formatted code after CI failure.

[JakeMerdichAMD]

After looking at more of the history (ie, the commit you referenced), I'd definitely be open to something like this, provided that it doesn't affect namespaces that reside completely on one line. Since it was mostly a clang-format limitation and relatively rare, I think we can change the default here, but that's not up to just me (+@MyDeveloperDay), and extra scrutiny is definitely required when changing existing tests.

In any case, can you also add the full diff context? It makes it easier for us to review.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

426–428

I strongly believe that these ones are not correct. Namespaces that are entirely on one line should never have a trailing comment, even if it has content in it.

A solution would also have to take into account whether future passes would split this onto separate lines (and thus have a different result after re-running clang-format), which was the reason for this limitation in the first place.

[kuzkry]

Thanks @JakeMerdichAMD for your quick feedback.

In any case, can you also add the full diff context? It makes it easier for us to review.

I'll do that starting from the next patch because I'm afraid if I resubmit the longer version of the same patch, your comment would be hidden or considered obsolete as it would refer to an older revision.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

426–428

You're absolutely right. Btw. do you happen to know which style option controls this behaviour that splits these namespaces onto separate lines?
Anyway, I'm going to look into this in a few days.

[MyDeveloperDay]

I have a hard time when people change tests! just because one person wants them this way doesn't mean everyone will.

I am not sure about others but I'd be ok about seeing this as an option to override the value but not changing the default.

I like to think of unit tests as the "Beyonce Rule", "if you liked it you put a test on it!"... when people come along and say... "All these tests are wrong"... I start thinking... is this subjective or objective?

The flux in llvm alone will be massive

https://github.com/llvm/llvm-project/search?q=namespace+%7B+++%7D&unscoped_q=namespace+%7B+++%7D

This would be a substantially smaller diff if you were adding the option. (option/doc/test...)

Full context diff please too!

clang/lib/Format/NamespaceEndCommentsFixer.cpp

28

I think change this will break the world... why don't we expose this as an option and not change everything!

[kuzkry]

Hey @MyDeveloperDay, thanks for the review.

On the one hand, you're right, it's a breaking change and I dislike it too.
But please, reconsider because on the other hand:
a) adding a new option means increasing our maintenance cost by possibly adding a rarely-used switch (which I also dislike)
b) it's been a part of clang-format documentation since release 5 so we could make clang-format work as expected
c) this patch doesn't have to be applied to already released versions and could be merged with a new major release. I think people can expect certain things to work differently while changing major versions.
d) in general, people care about preserving their git history and they normally don't format all their code blindly. I think the most frequent use case it to format only adjacent parts of code.

So, I'd say both ways have their ups and downs.

@all
Still, so far I've got two change requests from you lot but they are mutually exclusive.
To sum it up, which do you prefer?
a) add a new option (@MyDeveloperDay's approach)
b) change kShortNamespaceMaxLines (my approach) assuming @JakeMerdichAMD's suggestions (non-empty one-line namespaces aren't given the end comment)

[MyDeveloperDay]

Firstly let me say thank you for the patch, and taking the time to listen to the reviewers. Please don't be discouraged by my reply/opinon.

a) adding a new option means increasing our maintenance cost by possibly adding a rarely-used switch (which I also dislike)

I always here people complaining about this...but what is the cost to ALL the users that see formatting changes every release across all the projects that use clang-format. Its potentially massive. I'm not sure the cost is that high for us.

b) it's been a part of clang-format documentation since release 5 so we could make clang-format work as expected

Either the documentation isn't being read/understood/misinterpreted, or perhaps the person who wrote this wanted it like this, to not namespace comment small namespaces. Again who can tell, but in my view the precedent has been set for 6 releases and I'd be uncomfortable about changing it now without a get out clause if we were wrong,

if your view was correct then having an option and setting the default to the new value so people could set it back is a far better solution because it doesn't create an impasse for those that don't like your opinion of what is correct? but I think hundreds of projects will complain they have to change the default. (as many as complain they have to set it in the first place potentially)

c) this patch doesn't have to be applied to already released versions and could be merged with a new major release. I think people can expect certain things to work differently while changing major versions.

If its not good enough for trunk its not good enough for the next release in my view, there is never a good time to land something that causes a lot of flux. remember people massively lag the release number and often jump many version numbers, by which time going back and changing a default will be far too late.

I think people can expect certain things to work differently while changing major versions.

I think certain people expect things to work differently, others, easily an equal number expect it to just work as is, and don't want subjective changes.

d) in general, people care about preserving their git history and they normally don't format all their code blindly. I think the most frequent use case it to format only adjacent parts of code.

We should not make assumptions about what others care about.. IMHO this is not true, I work in an organization where any file that is committed MUST conform fully to clang-format resulting in failed phabricator builds (by design), every time we update clang-format we have to makes some alterations for bug fixes. But this change would likely cause a change in almost every file and the impact and cost would be massive to us alone let alone every project that might use it.

This is also what happens inside LLVM (in some sub-projects) where checking for clang-format status is actually part of what will be done via the build_bots. As soon as you check this in, you'll break LLVM, you need to be prepared for that!

I don't want to discourage you, and my opinion is as subjective as yours. But I'm afraid I won't be able to hit the accept button with the review in this form.

Feel free to reach out to other reviewers, mine is not the only opinion.

Ultimately thank you for the patch I think the idea behind it is a good change, I'd personally just want to give people the option, I see this as no different to the people who wanted Small Functions/If/Else blocks formatted differently if they were considered "Small"

Many thanks @MyDeveloperDay

[kuzkry]

Thanks @MyDeveloperDay, I appreciate your input in this review. You're a good reviewer. :)
Also, I think you've made some good points there and I'm going to turn this review into "non-breaking the world" one. I'll be back in a few days with a new diff.

[MyDeveloperDay]

In D87587#2276462, @kuzkry wrote:

Thanks @MyDeveloperDay, I appreciate your input in this review. You're a good reviewer. :)
Also, I think you've made some good points there and I'm going to turn this review into "non-breaking the world" one. I'll be back in a few days with a new diff.

Thank you for considering that, I'm pretty sure I for one will be happy to approve it in its new form. I think this has merits.

[kuzkry]

As proposed by @MyDeveloperDay, I added MaxUnwrappedLinesForShortNamespace option to control behaviour of FixNamespaceComments. This time I'm providing you with a full diff.
Btw. sorry for taking so long.

EDIT: git clang-format is unhappy but I think it's properly formatted. Please, run it locally to check what I mean.

[MyDeveloperDay]

Thanks for making the changes, this LGTM

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

2463

This is quite a mouthful before this lands do you want to consider shortening it?

ShortNamespaceLength ?

I'm not sure non clang-formast developers will know what an UnwrappedLine is?

[curdeius]

clang/docs/ClangFormatStyleOptions.rst

2463

I agree that one needs to wrap its head around to digest this name. I'd vote for what @MyDeveloperDay proposed.

[HazardyKnusperkeks]

clang/docs/ClangFormatStyleOptions.rst

2463

Although I already made some changes inside clang-format and single stepped through some formations, I still can not explain what those UnwrappedLines are.

[curdeius]

clang/docs/ClangFormatStyleOptions.rst

2463

Just found a precedence for this: https://clang.llvm.org/extra/clang-tidy/checks/llvm-namespace-comment.html. This clang-tidy check uses ShortNamespaceLines for the very same thing so maybe we should match?

[kuzkry]

Renamed MaxUnwrappedLinesForShortNamespace to ShortNamespaceLines (thank you @MyDeveloperDay for the idea and @curdeius for mentioning clang-tidy). I haven't changed the description of this option so you might wanna complain on that. Tbh. English isn't my native language so I'll take your tips if you wanna help simplify it.

Luckily formatting is magically fixed and hopefully CI check's gonna be satisfied.

[HazardyKnusperkeks]

Apart from my comments, this looks good to me and I really wonder why it hasn't been pushed. (I would directly use it!)

clang/include/clang/Format/Format.h

2821

Could you sort this one alphabetically?

clang/lib/Format/Format.cpp

576 ↗

(On Diff #313218)

Dito

939 ↗

(On Diff #313218)

Dito

[curdeius]

Please regroup tests and add release notes.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

1126

This could be tested together with another test.

1129

In general, I think you can merge some test cases to match the current testing "style".

1141

From the tests one could imply that 0 means "never add comments", or "add comments for non empty namespaces". Please add a test with at least one line in the namespace.

1161

You should check that Style.ShortNamespaceLines is 1 here.

[kuzkry]

Rebased to the tip of the main branch, sorted code alphabetically as suggested by @HazardyKnusperkeks and applied almost all fixes proposed by @curdeius (entry in release notes + some changes in unit tests)

[kuzkry]

In D87587#2552232, @HazardyKnusperkeks wrote:

Apart from my comments, this looks good to me and I really wonder why it hasn't been pushed. (I would directly use it!)

I don't know, maybe noone needed this except for us :D

clang/include/clang/Format/Format.h

2821

Oops, I just sed'ed through it when renaming and forgot to sort it. Thx :)

[kuzkry]

In D87587#2552314, @curdeius wrote:

Please regroup tests and add release notes.

Regroup tests - by that I think you meant all your 4 remarks about the tests. So I've done 3/4 :D
Release notes - done

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

1126

I merged it with "OneUnwrappedLine_*" tests

1129

Sorry, I didn't understand what you meant so I skipped this one for now.

1161

My initial idea was to have a separated test that would validate ShortNamespaceLines defaulting to "1". If that test failed, someone would know why and they might think:
"Oh my, I think I broke something, let's see what. Oh, I see the test is called "DefaultsToOneUnwrappedLine" so I probably changed this default". Now, some example test, say "OneUnwrappedLine_AddsEndCommentForLongNamespace", does two things, which is a bit like trying to kill two birds with one stone making it, imho, a bit inferior.

[curdeius]

Almost there. I hope I'm clear this time on what I'd like to see.
Thanks for the release notes update.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

1162

Nit.

1201–1206

What I meant by "merging the tests together" is just writing:

TEST_F(ShortNamespaceLinesTest,
       MultipleUnwrappedLine) {
  auto Style = getLLVMStyle();
  Style.ShortNamespaceLines = 2u;

  EXPECT_EQ(...);

  EXPECT_EQ(...);
}

This will get rid of duplicated setup (even if small), and, what's more important, show the behaviour with the same style on different cases.

Or even, but I'm not forcing you to do so, having a single test case for all ShortNamespaceLinesTest.* tests. It groups the test for the same feature together.

[kuzkry]

Rebased to the tip of the main branch, merged similar tests (requested by @curdeius)

[kuzkry]

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

1162

Well, instead of using comments or giving it a separate test, I wanted to have a nice self-explanatory name.

[HazardyKnusperkeks]

Although I would put it in one test, this is fine by me.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

1201–1206

I would even go further and put the 0, 1, and 2 in one function.

[HazardyKnusperkeks]

Is this acceptable to the others? Do you need someone to push it?

[kuzkry]

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

[curdeius]

LGTM.

[HazardyKnusperkeks]

In D87587#2592605, @kuzkry wrote:

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

Please state the name and mail for the commit. I will push it for you.

[kuzkry]

In D87587#2592722, @HazardyKnusperkeks wrote:

In D87587#2592605, @kuzkry wrote:

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

Please state the name and mail for the commit. I will push it for you.

Name: Krystian Kuzniarek
Email: [e-mail]

[kuzkry]

Thank you for delivering it, @HazardyKnusperkeks.

One thing concerns me though. I'm not an author of changes done in clang/docs/ClangFormatStyleOptions.rst in lines 2372, 3069-3073, 3075, 3077, 3082-3086, 3088, 3093-3097, 3099, 3104-3108, and 3110-3111. Perhaps it's just a mistake (something might have slipped through git add). Please let me know.

[HazardyKnusperkeks]

In D87587#2597241, @kuzkry wrote:

Thank you for delivering it, @HazardyKnusperkeks.

One thing concerns me though. I'm not an author of changes done in clang/docs/ClangFormatStyleOptions.rst in lines 2372, 3069-3073, 3075, 3077, 3082-3086, 3088, 3093-3097, 3099, 3104-3108, and 3110-3111. Perhaps it's just a mistake (something might have slipped through git add). Please let me know.

I've just run dump_format_style.py, why the other lines were changed (or not correct before) I don't know, but this is the result of the official script.

If that bothers you I can revert and split it in two commits. But I don't think that's necessary as the real author of those lines is the script, not what's written in the git history.

[kuzkry]

In D87587#2602523, @HazardyKnusperkeks wrote:

In D87587#2597241, @kuzkry wrote:

Thank you for delivering it, @HazardyKnusperkeks.

One thing concerns me though. I'm not an author of changes done in clang/docs/ClangFormatStyleOptions.rst in lines 2372, 3069-3073, 3075, 3077, 3082-3086, 3088, 3093-3097, 3099, 3104-3108, and 3110-3111. Perhaps it's just a mistake (something might have slipped through git add). Please let me know.

I've just run dump_format_style.py, why the other lines were changed (or not correct before) I don't know, but this is the result of the official script.

If that bothers you I can revert and split it in two commits. But I don't think that's necessary as the real author of those lines is the script, not what's written in the git history.

Oh, no need to do that. Thank you for your involvement in this review.

clang/docs/ClangFormatStyleOptions.rst

 
   NOTE: This is an experimental flag, that might go away or be renamed. Do
   not use this in config files, etc. Use at your own risk.
 
 **FixNamespaceComments** (``bool``)
   If ``true``, clang-format adds missing namespace end comments and
   fixes invalid existing ones.
 
+  Short namespaces will not be given an end comment, unless the comment
+  is invalid in which case it is always fixed.
+
   .. code-block:: c++
 
      true:                                  false:
      namespace a {                  vs.     namespace a {
      foo();                                 foo();
      } // namespace a                       }
 
 **ForEachMacros** (``std::vector<std::string>``)

clang/include/clang/Format/Format.h

   /// the input file and act accordingly.
   ///
   /// NOTE: This is an experimental flag, that might go away or be renamed. Do
   /// not use this in config files, etc. Use at your own risk.
   bool ExperimentalAutoDetectBinPacking;
 
   /// If ``true``, clang-format adds missing namespace end comments and
   /// fixes invalid existing ones.
+  ///
+  /// Short namespaces will not be given an end comment, unless the comment
+  /// is invalid in which case it is always fixed.
   /// \code
   ///    true:                                  false:
   ///    namespace a {                  vs.     namespace a {
   ///    foo();                                 foo();
   ///    } // namespace a                       }
   /// \endcode
   bool FixNamespaceComments;
 

clang/lib/Format/NamespaceEndCommentsFixer.cpp

 #define DEBUG_TYPE "namespace-end-comments-fixer"
 
 namespace clang {
 namespace format {
 
 namespace {
 // The maximal number of unwrapped lines that a short namespace spans.
 // Short namespaces don't need an end comment.
-static const int kShortNamespaceMaxLines = 1;
+static const int kShortNamespaceMaxLines = 0;
 
 // Computes the name of a namespace given the namespace token.
 // Returns "" for anonymous namespace.
 std::string computeName(const FormatToken *NamespaceTok) {
   assert(NamespaceTok &&
          NamespaceTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
          "expecting a namespace token");
   std::string name = "";

clang/unittests/Format/FormatTest.cpp

                    " int i;\n"
                    "\n"
                    "};"));
 
   // Don't remove empty lines at the start of namespaces or extern "C" blocks.
   EXPECT_EQ("namespace N {\n"
             "\n"
             "int i;\n"
-            "}",
+            "}  // namespace N",
             format("namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
   EXPECT_EQ("/* something */ namespace N {\n"
             "\n"
             "int i;\n"
-            "}",
+            "}  // namespace N",
             format("/* something */ namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
   EXPECT_EQ("inline namespace N {\n"
             "\n"
             "int i;\n"
-            "}",
+            "}  // namespace N",
             format("inline namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
   EXPECT_EQ("/* something */ inline namespace N {\n"
             "\n"
             "int i;\n"
-            "}",
+            "}  // namespace N",
             format("/* something */ inline namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
   EXPECT_EQ("export namespace N {\n"
             "\n"
             "int i;\n"
-            "}",
+            "}  // namespace N",
             format("export namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
   EXPECT_EQ("extern /**/ \"C\" /**/ {\n"
             "\n"
             "int i;\n"

             "};",
             format("namespace {\n"
                    "int i;\n"
                    "};",
                    LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
             "int i;\n"
             "\n"
-            "}",
+            "} // namespace",
             format("namespace {\n"
                    "int i;\n"
                    "\n"
                    "}"));
   EXPECT_EQ("namespace {\n"
             "int i;\n"
             "\n"
             "} // namespace",

   EXPECT_EQ("void f() { return }", format("void  f ( )  {  return  }"));
   EXPECT_EQ("void f() {\n"
             "  if (a)\n"
             "    return\n"
             "}",
             format("void  f  (  )  {  if  ( a )  return  }"));
   EXPECT_EQ("namespace N {\n"
             "void f()\n"
-            "}",
+            "} // namespace N",
             format("namespace  N  {  void f()  }"));
   EXPECT_EQ("namespace N {\n"
             "void f() {}\n"
             "void g()\n"
             "} // namespace N",
             format("namespace N  { void f( ) { } void g( ) }"));
 }
 

                "{};",
                Style);
   verifyFormat("export namespace Foo\n"
                "{};",
                Style);
   verifyFormat("namespace Foo\n"
                "{\n"
                "void Bar();\n"
-               "};",
+               "}; // namespace Foo",
                Style);
 }
 
 TEST_F(FormatTest, NeverMergeShortRecords) {
   FormatStyle Style = getLLVMStyle();
 
   verifyFormat("class Foo {\n"
                "  Foo();\n"

                "};",
                Style);
   verifyFormat("typedef union Foo {\n"
                "  A,\n"
                "} Foo_t;",
                Style);
   verifyFormat("namespace Foo {\n"
                "void Bar();\n"
-               "};",
+               "}; // namespace Foo",
                Style);
 
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterClass = true;
   Style.BraceWrapping.AfterStruct = true;
   Style.BraceWrapping.AfterUnion = true;
   Style.BraceWrapping.AfterNamespace = true;
   verifyFormat("class Foo\n"

   verifyFormat("typedef union Foo\n"
                "{\n"
                "  A,\n"
                "} Foo_t;",
                Style);
   verifyFormat("namespace Foo\n"
                "{\n"
                "void Bar();\n"
-               "};",
+               "}; // namespace Foo",
                Style);
 }
 
 TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) {
   // Elaborate type variable declarations.
   verifyFormat("struct foo a = {bar};\nint n;");
   verifyFormat("class foo a = {bar};\nint n;");
   verifyFormat("union foo a = {bar};\nint n;");

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

             "}// namespace ::A::B",
             fixNamespaceEndComments("namespace /**/::/**/A/**/::/**/B/**/ {\n"
                                     "int i;\n"
                                     "int j;\n"
                                     "}"));
   EXPECT_EQ("namespace A {\n"
             "namespace B {\n"
             "int i;\n"
-            "}\n"
+            "}// namespace B\n"
             "}// namespace A",
             fixNamespaceEndComments("namespace A {\n"
                                     "namespace B {\n"
                                     "int i;\n"
                                     "}\n"
                                     "}"));
   EXPECT_EQ("namespace A {\n"
             "namespace B {\n"

                                     "int i;\n"
                                     "int j;\n"
                                     "};;"));
 }
 
 TEST_F(NamespaceEndCommentsFixerTest, DoesNotAddEndCommentForShortNamespace) {
   EXPECT_EQ("namespace {}", fixNamespaceEndComments("namespace {}"));
   EXPECT_EQ("namespace A {}", fixNamespaceEndComments("namespace A {}"));
-  EXPECT_EQ("namespace A { a }", fixNamespaceEndComments("namespace A { a }"));
-  EXPECT_EQ("namespace A { a };",
+  EXPECT_EQ("namespace A { a }// namespace A",
+            fixNamespaceEndComments("namespace A { a }"));
+  EXPECT_EQ("namespace A { a };// namespace A",
             fixNamespaceEndComments("namespace A { a };"));
 }
 
 TEST_F(NamespaceEndCommentsFixerTest, DoesNotAddCommentAfterUnaffectedRBrace) {
   EXPECT_EQ("namespace A {\n"
             "int i;\n"
             "}",
             fixNamespaceEndComments("namespace A {\n"