On Mon, Jun 22, 2015 at 3:38 PM, Alexander Kornienko <ale...@google.com> wrote:
> > On Mon, Jun 22, 2015 at 6:15 PM, David Blaikie <dblai...@gmail.com> wrote: > >> >> >> On Mon, Jun 22, 2015 at 2:47 AM, Alexander Kornienko <ale...@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. >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
