On Tue, Jun 23, 2015 at 12:52 AM, David Blaikie <[email protected]> wrote:
> > > On Mon, Jun 22, 2015 at 3:38 PM, Alexander Kornienko <[email protected]> > wrote: > >> >> On Mon, Jun 22, 2015 at 6:15 PM, David Blaikie <[email protected]> >> wrote: >> >>> >>> >>> On Mon, Jun 22, 2015 at 2:47 AM, Alexander Kornienko <[email protected]> >>> 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. > > >> 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 [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
