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

[Hal Finkel]

----- Original Message -----
> From: "Alexander Kornienko" <alexfh at google.com>
> To: "David Blaikie" <dblaikie at gmail.com>
> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>
> Sent: Monday, June 22, 2015 5:38:59 PM
> Subject: Re: r240270 - Fixed/added namespace ending comments using clang-tidy.	NFC
> 
> 
> 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 , 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?
> 
> 
> Also, what the preferred solution here would be?
> 
> 
> 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
> 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
> c. // end namespace llvm
> d. // end of namespace llvm
> e. // end llvm namespace
> f. something else
> 

Also, I prefer that anonymous namespaces are explicitly commented as such if they're going to be commented, and we also don't generally add indentation for anonymous namespaces. This change added comments like:

} // namespace

and I'd rather it said:

// anonymous namespace

Regarding the actual format, putting 'end %s namespace' seems to be preferred:

$ grep -r 'anonymous namespace' lib | sed 's/.*://' | sed 's/[ ]\+/ /g' | sort | uniq -c
     16 } // anonymous namespace
      2 } // anonymous namespace.
    134 } // end anonymous namespace
     15 } // end anonymous namespace.
      2 } //end anonymous namespace
     18 } // End anonymous namespace
     11 } // End anonymous namespace.
      1 } //End anonymous namespace
     13 } // end of anonymous namespace
      8 } // End of anonymous namespace
      1 } // End of anonymous namespace.

 -Hal

> 
> This patch corresponds to 4b.
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory