r240270 - Fixed/added namespace ending comments using clang-tidy. NFC

[Alexander Kornienko]

On Tue, Jun 23, 2015 at 12:52 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Mon, Jun 22, 2015 at 3:38 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
>
>>
>> On Mon, Jun 22, 2015 at 6:15 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Mon, Jun 22, 2015 at 2:47 AM, Alexander Kornienko <alexfh at google.com>
>>> wrote:
>>>
>>>> Author: alexfh
>>>> Date: Mon Jun 22 04:47:44 2015
>>>> New Revision: 240270
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=240270&view=rev
>>>> Log:
>>>> Fixed/added namespace ending comments using clang-tidy. NFC
>>>>
>>>
>>> Is this actually an LLVM or Clang coding convention? It doesn't seem to
>>> be & I'm not sure it's one worth adopting... might be worth some discussion
>>> before we go in this direction? (perhaps there has been discussion in a
>>> review thread, etc & I've missed it - helpful to call that out in the
>>> commit message (& probably put this in the LLVM coding conventions if we
>>> are going in that direction))
>>>
>>
>> I was having an impression that there's a convention to use namespace
>> ending comments of this form, that's why I considered a purely mechanical
>> change that doesn't need a discussion. However, now reading the coding
>> standards
>> <http://llvm.org/docs/CodingStandards.html#namespace-indentation>, I see
>> that the relevant rule is much weaker, it just allows adding comments, not
>> even recommends them:
>>
>> If it helps readability, feel free to add a comment indicating what
>> namespace is being closed by a }.
>>
>>
>> and also the style of the comment used in the example is slightly
>> different:
>>
>> } // end namespace llvm
>>
>> instead of
>>
>> } // namespace llvm
>>
>> used in this patch.
>>
>> So an agreement on the style (and necessity) of the namespace ending
>> comments prior to committing the patch wouldn't hurt. Would you like me to
>> revert the patches before we figure out what the desired state is?
>>
>
> I think that'd be the best idea, yes (revert, discuss, then commit
> whatever ends up being decided on).
>

Reverted this patch in r240353. Will revert the other two patches
separately.


>
>
>> Also, what the preferred solution here would be?
>>
>
> Probably part of the discussion we can have on llvm-dev about this.
>
>
>>   1. leave the comments (or the lack thereof) alone, don't even fix
>> incorrect or inconsistent ones
>>   2. only fix incorrect comments
>>   3. fix incorrect comments and make all existing namespace ending
>> comments consistent
>>
>
> I'd probably go with (3) here, but there may be other ideas/perspectives,
> prefer a more "do this everywhere/lots of places (4)", etc.
>
>
>>   4. 3. + add namespace ending comments to all namespaces spanning more
>> than X lines
>>   5. something else?
>>
>> In cases 2-4 we need to decide which format of namespace ending comments
>> to use:
>>   a. // llvm
>>   b. // namespace llvm
>>
>
> I'd probably choose (b) but don't mind (c) but that's a change to the
> style - easier to justify now that we have tools to automate this cleanup.
>
> I guess if I were picking the comment out of thin air I might say "llvm
> namespace".
>
>
>>   c. // end namespace llvm
>>   d. // end of namespace llvm
>>   e. // end llvm namespace
>>   f. something else
>>
>> This patch corresponds to 4b.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150623/9c121deb/attachment.html>