On Tue, Jun 23, 2015 at 1:14 AM, Alexander Kornienko <ale...@google.com> wrote:
> > > On Tue, Jun 23, 2015 at 12:52 AM, David Blaikie <dblai...@gmail.com> > wrote: > >> >> >> 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). >> > > 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. >>> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
