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

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/6edd0444/attachment.html>

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>