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

[David Blaikie]

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


> 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/20150622/680c2670/attachment.html>