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

[Carlos Galvez via llvm-dev]

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 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 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

[David Blaikie via llvm-dev]

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)

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Keane, Erich via llvm-dev]

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)

[James Y Knight via llvm-dev]

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.

[Philip Reames via llvm-dev]

I agree with James.  Both are reasonable, this doesn't really matter, we don't have to pick and enforce one.

Philip

[Geoffrey Martin-Noble via llvm-dev]

I think James basically said the opposite: that we should use whatever clang-format does and be done with it. Things that don't really matter are one of the best cases for a style guide (accompanied by tooling to do it automatically). Otherwise you get reviewers commenting as they did on Carlos' patch. I would recommend that we encourage people to not comment on such things, but having it tool enforced and consistent seems good.

[Geoffrey Martin-Noble via llvm-dev]

And slightly different numbers (though I think it doesn't change any conclusions):

$ git grep -E '}\s+//\s+namespace' | wc -l
14030

$ git grep -E '}\s+//\s+end\s+namespace' | wc -l
4185

[Philip Reames via llvm-dev]

Er, you and I are reading James' response very differently. 

To me, this topic does not matter.  We do not need to enforce a rule on this.  Reviewers should not be wasting time on this. 

Philip

[Geoffrey Martin-Noble via llvm-dev]

I'm referring specifically to this part

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

But I'm not sure the key question here is what James said :-P I agree that reviewers should not pay any attention to this, but it would be nice if the coding guidelines conformed to whatever clang-format does (which doesn't even need to be explicit for this rule. It can just be a catch-all "do what clang-format says"). I think the coding examples are non-normative, so *shouldn't* be used as evidence of unrelated rules, but obviously people do actually read them that way, so formatting them with clang-format seems like a good idea.

[Salman Javed via llvm-dev]

On 12/6/21 12:47 PM, James Y Knight via llvm-dev wrote:
> That said, we should update the coding guidelines to recommend the
> format which clang-tidy emits -- just to make everyone's lives easier.

As a newcomer to this project, this would make my life easier. Before
submitting a patch for a review, I want to have some confidence that I
haven't made some rookie mistake (missing namespace closing comments
being a perfect example), so naturally I will run clang-tidy and
clang-format. Then the reviewer can spend more time looking at the meat
of my patch, rather than acting like a kind of human
clang-format/clang-tidy pre-merge tool.

If the coding guidelines document defaulted to clang-tidy's suggested
namespace comment style, that would cause the least amount of confusion
for me. I will follow that style for any new namespaces I introduce to
the code base.

I wouldn't go out and proclaim that all existing code that uses the
other style ("end namespace clang") is now in direct violation of the
guidelines and must be changed, though.

[Carlos Galvez via llvm-dev]

In case it helps, this graph shows the actual distribution of both styles (in %), using the regex suggested by Geoffrey above. I personally thing this is just something clang-format should update automatically as files get modified - over time the style automatically becomes consistent. But that's another story, I'm mostly interested in how to improve the Coding Guidelines at this point.

Best regards,

Carlos

[MyDeveloper Day via llvm-dev]

Nice chart.. (love a picture!)

If we allow the systematic clang-formatting all the code, I'd be happy to make alterations to clang-format to allow a switch of

// end of namspace X

// end namespace X

to 

// namespace X

(which is currently allows any of all 3 forms)

However at present we have enough problems getting the % of clang-formatted files in LLVM/Clang above 50% (including excluding lit tests), making such a change will likely reduce our overall clang-format clean status by 10% and we use these "clang-formatted"  clean files as a regression suite for clang-format to ensure we are not breaking functionality. (which protects us and ultimately you).

https://clang.llvm.org/docs/ClangFormattedStatus.html

As for the RFC I totally agree with aligning the documentation to what clang-format does by default.

MyDeveloperDay.

[Renato Golin via llvm-dev]

On Tue, 7 Dec 2021 at 10:07, Carlos Galvez via llvm-dev <llvm...@lists.llvm.org> wrote:

I personally thing this is just something clang-format should update automatically as files get modified - over time the style automatically becomes consistent.

No, clang-format shouldn't be changing the actual contents as much as it can, especially if clang-tidy does the same job.

IIUC, James proposal was to match the documentation with whatever clang-tidy emits, whichever way it goes. 

The meaning of all versions are exactly the same and it doesn't really matter which way you have it.

I echo some comments here that we should focus on bigger things.

The only action I would take here is to update either the docs, or clang-tidy, to match each other and the general current preference, which seems to be "// namespace" (the blue bars).

I would strongly discourage people patching existing sources to match the style just for the sake of style. This is one of those things that aren't worth having a patch just for this.

cheers,

--renato

[Carlos Galvez via llvm-dev]

> especially if clang-tidy does the same job.

Thanks, I forgot clang-tidy also enforces this. I've played with it and it will change the existing style if you have a typo on the namespace.

> This is one of those things that aren't worth having a patch just for this.

Fully agree, I meant as part of the pre-commit "git clang-format" that we anyway do on regular patches. But I can see how even that can cause significant noise during review.

So to summarize the comments so far:

• Most predominant style is "// namespace".
• Style in Coding Guidelines should be consistent with clang-format and clang-tidy.
• Regardless of what it says in the Coding Guidelines, both styles are OK and achieve the same goal -> reviewers should not request changes here.
• It's not worth changing existing code.
  

Based on that, would you be OK with proceeding with the patch to update the examples in the Coding Guidelines? If not, do you have any suggestions on what should be improved, or should I just drop it altogether?

Best regards,

Carlos

[James Y Knight via llvm-dev]

I accepted the revision. Please submit, so we can put this issue to rest. :)

_______________________________________________

[Carlos Galvez via llvm-dev]

Pushed now, thanks everyone for your time!

[Chris Lattner via llvm-dev]

Thank you for driving this Carlos!

-Chris