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

Tue Dec 7 02:20:18 PST 2021

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.

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

> 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.
> [image: llvm_style.png]
> Best regards,
> Carlos
>
> On Tue, Dec 7, 2021 at 12:05 AM Geoffrey Martin-Noble <gcmn at google.com>
> wrote:
>
>> 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.
>>
>> On Mon, Dec 6, 2021 at 2:59 PM Philip Reames <listmail at philipreames.com>
>> wrote:
>>
>>> 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
>>> On 12/6/21 2:04 PM, Geoffrey Martin-Noble wrote:
>>>
>>> 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.
>>>
>>> On Mon, Dec 6, 2021 at 1:31 PM Philip Reames via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> I agree with James.  Both are reasonable, this doesn't really matter,
>>>> we don't have to pick and enforce one.
>>>>
>>>> Philip
>>>> On 12/6/21 12:47 PM, James Y Knight via llvm-dev wrote:
>>>>
>>>> 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 -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
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> llvm-dev at lists.llvm.org
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing listllvm-dev at lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>> _______________________________________________
> 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/20211207/de857a3f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_style.png
Type: image/png
Size: 29802 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/de857a3f/attachment.png>