llvm dev - Dec 2021 - [docs][RFC] Style for "end namespace" comments

Hi,

I was recently working on a patch and was asked during review to replace
existing:
"// end namespace clang"   Style A
with :
"// namespace clang"          Style B

After that, I got interested to understand what the preferred style is, and
found in the Coding Guidelines
<https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions>
that the style is actually Style A.

On the other hand, clang-format will automatically enforce Style B on new
code, via the FixNamespaceComments option, which is set to "true" for
the
LLVM style. clang-format will keep the Style A if it already exists,
however. Most people using clang-format (outside LLVM) will probably be
more familiar with Style B.

Additionally, I have seen the following usage numbers in the repo:

$ git grep '//

* end' | wc -l6724$ git grep '//* namespace' | wc -l
14348

So Style B seems to be more adopted. Therefore I wanted to ask - should we
update the Coding Guidelines to reflect this, and avoid these kinds of
style discussions in code reviews? If so, what style should be preferred? I
have a patch <https://reviews.llvm.org/D115115> for review and there seems
to be a preference for keeping both styles. Regardless of the choice, I
don't think this should lead to an urgent style change of the whole
codebase.

Looking forward to your feedback!

Best regards,
Carlos
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211206/7804dd2c/attachment.html>

Do the grep results seem to be the intended ones? (or are some of the
comments unrelated to end-of-namespace comments that might then skew the
data incorrectly?)

If so, then I think updating the coding guidelines to match predominant
existing practice seems suitable. (maybe double check if there's been any
history of that rule in the coding guidelines - maybe it used to be the
"namespace" version and then changed, so perhaps we have a lot of
outdated
comments using a prior style? This is certainly true of naming, but I guess
it's probably not true for this instance but could be worth checking)

On Mon, Dec 6, 2021 at 12:04 PM Carlos Galvez via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> I was recently working on a patch and was asked during review to replace
> existing:
> "// end namespace clang"   Style A
> with :
> "// namespace clang"          Style B
>
> After that, I got interested to understand what the preferred style is,
> and found in the Coding Guidelines
>
<https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions>
> that the style is actually Style A.
>
> On the other hand, clang-format will automatically enforce Style B on new
> code, via the FixNamespaceComments option, which is set to "true"
for the
> LLVM style. clang-format will keep the Style A if it already exists,
> however. Most people using clang-format (outside LLVM) will probably be
> more familiar with Style B.
>
> Additionally, I have seen the following usage numbers in the repo:
>
> $ git grep '//
>
> * end' | wc -l6724$ git grep '//* namespace' | wc -l
> 14348
>
> So Style B seems to be more adopted. Therefore I wanted to ask - should we
> update the Coding Guidelines to reflect this, and avoid these kinds of
> style discussions in code reviews? If so, what style should be preferred? I
> have a patch <https://reviews.llvm.org/D115115> for review and there
> seems to be a preference for keeping both styles. Regardless of the choice,
> I don't think this should lead to an urgent style change of the whole
> codebase.
>
> Looking forward to your feedback!
>
> Best regards,
> Carlos
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211206/f8a04943/attachment.html>

From my reading of the coding-standard (AND your patch!) I don’t see where it
even requires either form, other than ‘thou shall run clang-format’.  Use in
not-particularly-related examples doesn’t have a ‘shall’ relationship for me at
all.  For example, nothing in that Use Namespace Qualifiers… should convince you
to use functions starting with ‘lower case’ (as in ‘foo’).

IMO, I think if we want to say anything, it should be that we want to have
end-of-namespace curleys marked, either as // namespace llvm (or just //
namespace?)  or // end namespace llvm (or //end anonymous namespace)

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Carlos
Galvez via llvm-dev
Sent: Monday, December 6, 2021 11:16 AM
To: llvm-dev at lists.llvm.org
Subject: [llvm-dev] [docs][RFC] Style for "end namespace" comments

Hi,

I was recently working on a patch and was asked during review to replace
existing:
"// end namespace clang"   Style A
with :
"// namespace clang"          Style B

After that, I got interested to understand what the preferred style is, and
found in the Coding
Guidelines<https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions>
that the style is actually Style A.

On the other hand, clang-format will automatically enforce Style B on new code,
via the FixNamespaceComments option, which is set to "true" for the
LLVM style. clang-format will keep the Style A if it already exists, however.
Most people using clang-format (outside LLVM) will probably be more familiar
with Style B.

Additionally, I have seen the following usage numbers in the repo:

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

So Style B seems to be more adopted. Therefore I wanted to ask - should we
update the Coding Guidelines to reflect this, and avoid these kinds of style
discussions in code reviews? If so, what style should be preferred? I have a
patch<https://reviews.llvm.org/D115115> for review and there seems to be a
preference for keeping both styles. Regardless of the choice, I don't think
this should lead to an urgent style change of the whole codebase.

Looking forward to your feedback!

Best regards,
Carlos
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211206/4e748d86/attachment.html>

Both styles accomplish the goal of annotating what namespace is being
closed -- and both are widely used within the codebase. So I think there's
not an intrinsic reason to prefer one over the other. They're both fine.

That said, we should update the coding guidelines to recommend the format
which clang-tidy emits -- just to make everyone's lives easier.



On Mon, Dec 6, 2021 at 3:03 PM Carlos Galvez via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> I was recently working on a patch and was asked during review to replace
> existing:
> "// end namespace clang"   Style A
> with :
> "// namespace clang"          Style B
>
> After that, I got interested to understand what the preferred style is,
> and found in the Coding Guidelines
>
<https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions>
> that the style is actually Style A.
>
> On the other hand, clang-format will automatically enforce Style B on new
> code, via the FixNamespaceComments option, which is set to "true"
for the
> LLVM style. clang-format will keep the Style A if it already exists,
> however. Most people using clang-format (outside LLVM) will probably be
> more familiar with Style B.
>
> Additionally, I have seen the following usage numbers in the repo:
>
> $ git grep '//
>
> * end' | wc -l6724$ git grep '//* namespace' | wc -l
> 14348
>
> So Style B seems to be more adopted. Therefore I wanted to ask - should we
> update the Coding Guidelines to reflect this, and avoid these kinds of
> style discussions in code reviews? If so, what style should be preferred? I
> have a patch <https://reviews.llvm.org/D115115> for review and there
> seems to be a preference for keeping both styles. Regardless of the choice,
> I don't think this should lead to an urgent style change of the whole
> codebase.
>
> Looking forward to your feedback!
>
> Best regards,
> Carlos
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211206/06bd5551/attachment.html>