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

[Alexander Kornienko]

On Tue, Jun 23, 2015 at 1:14 AM, Alexander Kornienko <alexfh at google.com>
wrote:

>
>
> 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.
>

The other two reverted in r240390 and r240393.


>
>
>>
>>
>>> 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/5d6373b6/attachment.html>