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:
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.
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)
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)
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.
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.
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 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.
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.
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.
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).
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.
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?
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.
