[doc] Fix namespace comment style in Coding Guidelines

[carlosgalvezp]

Summary

The Coding Guidelines specify that the ending brace of a
namespace shall have a comment like:

} // end namespace clang

However the majority of the code uses a different style:

} // namespace clang

Indeed:

$ git grep ' end' | wc -l
6724
$ git grep ' namespace' | wc -l
14348

Besides, this is the style enforced automatically by clang-format,
via the FixNamespaceComments option.

Having inconsistencies between the Coding Guidelines and the
code/tooling creates confusion, can lead to bikeshedding during
reviews and overall delays merging code. Therefore, update the
guidelines to reflect current usage. Updating legacy code to the
new standard should be done in a separate patch, if wanted.

[salman-javed-nz]

The ~6700 instances of // end namespace - do you observe any trend, e.g. are they concentrated in one of the LLVM sub-projects? Is it similar to the situation we have with the readability-identifier-naming check, where a number of sub-projects have disabled it in their sub-directory's .clang-tidy file?

[carlosgalvezp]

In D115115#3172329, @salman-javed-nz wrote:

The ~6700 instances of // end namespace - do you observe any trend, e.g. are they concentrated in one of the LLVM sub-projects? Is it similar to the situation we have with the readability-identifier-naming check, where a number of sub-projects have disabled it in their sub-directory's .clang-tidy file?

I see them pretty much in all sub-projects, mostly in clang and llvm due to them being the bigger sub-projects. But both styles are used on all sub-projects (the latter style used more frequently).

Unlike .clang-tidy files, I cannot see any .clang-format that sets FixNamespaceComments = false. The clang-format LLVM style hardcodes the value to true:

clang/lib/Format/Format.cpp:  LLVMStyle.FixNamespaceComments = true;

This initiative comes from a previous review where I got feedback that the file I was updating (GlobList.h I think) had "incorrect" namespace comments, so I fixed it. Now I looked at the whole clang-tidy folder and noticed the problem exists in a few more files (not in the checkers, only in the "infrastructure" code). So this got me wondering and thought it would be good to read the Coding Guidelines, and sure enough the old style is there. Since it seems people consider that style "wrong" I have decided to put up this patch to update, so that we don't run into discussions like "I know this style is wrong, but the Coding Guidelines say otherwise".

Probably clang-format doesn't fix existing comments, that's why they remain. My guess as a newcomer is that first there was the old style, then clang-format came and set the new style, while not fixing the old style automatically.

[carlosgalvezp]

Btw if you know the right person to review these kinds of changes please feel free to add them :)

[MyDeveloperDay]

FYI clang-format will correct namespace comments if you change the namespace name, or add some other comment there

However likely for historical reasons we don't change

// end namespace <name>
// end of namespace <name>

[carlosgalvezp]

That explains, thanks for the info! Would it make sense to update clang-format to replace the old style? (if we agree with this patch, of course). Then files could get the namespace fixed over time as they get updated.

[MyDeveloperDay]

I'm not a massive fan of changing the defaults of clang-format because this can have an impact on anyone using LLVM as a style base (a lot of people outside of LLVM)
, and we will massively break our https://clang.llvm.org/docs/ClangFormattedStatus.html

If I thought it was ok for me to spin through all the LLVM code and clang-format everything then I'd say yes, but this was decided we didn't want to do that. (Although I would love to start chipping away at those files that are 99% as often they lose their clean status from time to time)

[carlosgalvezp]

Makes sense, quite a big change for little benefit, perhaps. The scope of this patch is the Coding Guidelines only, however. Do you see any reason not to update this to reflect current usage? People in code reviews are pointing at old style requesting to change it to the new style, and clang-format by default will use the new style on new code, so we might as well make it "official" in the guidelines?

[MyDeveloperDay]

I think your change mirrors what clang-format does automatically so I personally agree with it.

[carlosgalvezp]

Thanks! Do you know who should I tag as reviewer that has "authority" to approve the patch? I checked in CODE_OWNERS.txt but couldn't find anyone in charge of docs...

[MyDeveloperDay]

In D115115#3173295, @carlosgalvezp wrote:

Thanks! Do you know who should I tag as reviewer that has "authority" to approve the patch? I checked in CODE_OWNERS.txt but couldn't find anyone in charge of docs...

I tend to look back at previous reviewers for files I don't normally edit, @sammccall , @MaskRay , @RKSimon , @dexonsmith , @rnk , these people have reviewed it before and have more clout than me, but I'm happy to LGTM from the aspect of keeping in line with what clang-format does by default. (which is why I saw this in the first place).

If you get no response I'm happy to come back and accept once we've given them time to take a look, at that point we can seek forgiveness rather than permission.

[aaron.ballman]

Given that both forms appear in the source code, the goal of the style guide rule here is to ensure it's clear that the reader knows which namespace is being closed, and that either form achieves that goal... should we bless both forms in the coding standard so nobody comes back and says "you have to change this"?

(My search for // end namespace brings up 1076 entries and // namespace brings up 2569 entries (both just searching over Clang), so it's unlikely we're ever going to get perfect consistency here anyway.)

In D115115#3173295, @carlosgalvezp wrote:

Thanks! Do you know who should I tag as reviewer that has "authority" to approve the patch? I checked in CODE_OWNERS.txt but couldn't find anyone in charge of docs...

Pretty much any change to CodingStandards.rst should go through a wider community review anyway because there's not really one or two people who can make decisions for the whole community like that. I'd recommend an RFC (as annoying as those can be) to make sure there's consensus before moving forward. Once the community shows they're not strongly opposed to the change, then any trusted reviewer(s) can sign off on it because they can check the patch against the community intent.

[MaskRay]

Agree that an RFC applies and supporting both (end marker) comment styles looks good to me.

I suspect end namespace may be some editor/configuration's preference but llvm/utils/ does not contain the string end namespace.

[carlosgalvezp]

My search for // end namespace brings up 1076 entries

Actually if you search for // end you'll get 1750+ hits, due to // end anonymous namespace :) Then it's a more even split, so perhaps it does makes sense to keep both.

I'd recommend an RFC.

Sounds good! What list should I use, would llvm-dev be appropriate?

[aaron.ballman]

In D115115#3174158, @carlosgalvezp wrote:

My search for // end namespace brings up 1076 entries

Actually if you search for // end you'll get 1750+ hits, due to // end anonymous namespace :) Then it's a more even split, so perhaps it does makes sense to keep both.

I'd recommend an RFC.

Sounds good! What list should I use, would llvm-dev be appropriate?

Yeah, I'd send it to llvm-dev initially. We can always widen the audience if we think that's helpful.

[jyknight]

Correcting the examples to match the style emitted automatically by our tooling is an improvement.

I believe there's overall support for this on the mailing-list thread (in between annoyance at anyone wasting time on this issue), so accepting the CL.

[carlosgalvezp]

Thanks for your time!

llvm/docs/CodingStandards.rst

 
 .. code-block:: c++
 
   // Foo.cpp
   #include "Foo.h"
   namespace llvm {
   int foo(char *s) { // Mismatch between "const char *" and "char *"
   }
-  } // end namespace llvm
+  } // namespace llvm
 
 This error will not be caught until the build is nearly complete, when the
 linker fails to find a definition for any uses of the original function.  If the
 function were instead defined with a namespace qualifier, the error would have
 been caught immediately when the definition was compiled.
 
 Class method implementations must already name the class and new overloads
 cannot be introduced out of line, so this recommendation does not apply to them.
.. code-block:: c++
   public:
     explicit Grokable() { ... }
     virtual ~Grokable() = 0;
 
     ...
 
   };
 
-  } // end namespace knowledge
-  } // end namespace llvm
+  } // namespace knowledge
+  } // namespace llvm
 
 
 Feel free to skip the closing comment when the namespace being closed is
 obvious for any reason. For example, the outer-most namespace in a header file
 is rarely a source of confusion. But namespaces both anonymous and named in
 source files that are being closed half way through the file probably could use
 clarification.
 
.. code-block:: c++
 
   namespace {
   class StringSort {
   ...
   public:
     StringSort(...)
     bool operator<(const char *RHS) const;
   };
-  } // end anonymous namespace
+  } // namespace
 
   static void runHelper() {
     ...
   }
 
   bool StringSort::operator<(const char *RHS) const {
     ...
   }
 
 Avoid putting declarations other than classes into anonymous namespaces:
 
 .. code-block:: c++
 
   namespace {
 
   // ... many declarations ...
 
   void runHelper() {
     ...
   }
 
   // ... many declarations ...
 
-  } // end anonymous namespace
+  } // namespace
 
 When you are looking at "``runHelper``" in the middle of a large C++ file,
 you have no immediate way to tell if this function is local to the file.  In
 contrast, when the function is marked static, you don't need to cross-reference
 faraway places in the file to tell that the function is local.
 
 Don't Use Braces on Simple Single-Statement Bodies of if/else/loop Statements
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^