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

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


On Tue, Dec 7, 2021 at 6:52 AM Carlos Galvez via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> > 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 <https://godbolt.org/z/z6d5KnhaG> 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
> <https://reviews.llvm.org/D115115> 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
>
>
>
> On Tue, Dec 7, 2021 at 11:50 AM Renato Golin <rengolin at gmail.com>
wrote:
>
>> On Tue, 7 Dec 2021 at 10:07, Carlos Galvez via llvm-dev <
>> llvm-dev at 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
>>
> _______________________________________________
> 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/95fecc8b/attachment.html>

Pushed now, thanks everyone for your time!

On Tue, Dec 7, 2021 at 2:30 PM James Y Knight <jyknight at google.com>
wrote:
> I accepted the revision. Please submit, so we can put this issue to rest.
> :)
>
>
> On Tue, Dec 7, 2021 at 6:52 AM Carlos Galvez via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> > 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
<https://godbolt.org/z/z6d5KnhaG> 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
>> <https://reviews.llvm.org/D115115> 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
>>
>>
>>
>> On Tue, Dec 7, 2021 at 11:50 AM Renato Golin <rengolin at
gmail.com> wrote:
>>
>>> On Tue, 7 Dec 2021 at 10:07, Carlos Galvez via llvm-dev <
>>> llvm-dev at 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
>>>
>> _______________________________________________
>> 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/ec191963/attachment.html>