<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 23, 2015 at 1:14 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Jun 23, 2015 at 12:52 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Jun 22, 2015 at 3:38 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Jun 22, 2015 at 6:15 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 22, 2015 at 2:47 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: alexfh<br>
Date: Mon Jun 22 04:47:44 2015<br>
New Revision: 240270<br>
<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D240270-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=nIjlEXGnrXblAGxhUghQOHLYiegMBCQja8O2AuUfdD4&s=_Kn1GdJcw5e4tGRo8b5WWcu1pfiT-7AQsNFTV0QdcDY&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=240270&view=rev</a><br>
Log:<br>
Fixed/added namespace ending comments using clang-tidy. NFC<br></blockquote><div><br>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))<br></div></div></div></div></blockquote><div><br></div></span><div>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 <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_docs_CodingStandards.html-23namespace-2Dindentation&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=nIjlEXGnrXblAGxhUghQOHLYiegMBCQja8O2AuUfdD4&s=v07hoaJGYt5Kgj8ckFVH4l4hqp8rhJYOHTmSVZGQGBA&e=" target="_blank">coding standards</a>, I see that the relevant rule is much weaker, it just allows adding comments, not even recommends them:</div><div><br></div></div></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div class="gmail_extra"><div class="gmail_quote"><div><span style="color:rgb(0,0,0);font-family:'Lucida Grande','Lucida Sans Unicode',Geneva,Verdana,sans-serif;font-size:14px;line-height:21px">If it helps readability, feel free to add a comment indicating what namespace is being closed by a </span><tt style="font-family:Consolas,'Deja Vu Sans Mono','Bitstream Vera Sans Mono',monospace;font-size:0.95em;color:rgb(0,0,0);line-height:21px"><span>}</span></tt><span style="color:rgb(0,0,0);font-family:'Lucida Grande','Lucida Sans Unicode',Geneva,Verdana,sans-serif;font-size:14px;line-height:21px">.</span><br></div></div></div></blockquote> <div>and also the style of the comment used in the example is slightly different:<br></div><div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div><pre style="font-family:Consolas,'Deja Vu Sans Mono','Bitstream Vera Sans Mono',monospace;font-size:0.95em;line-height:15.960000038147px;padding:0.5em;border:1px solid rgb(204,204,204);color:rgb(0,0,0);background-color:rgb(248,248,248)"><span style="color:rgb(96,160,176);font-style:italic">} // end namespace llvm</span></pre></div></blockquote></div><div>instead of</div><div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div><pre style="font-family:Consolas,'Deja Vu Sans Mono','Bitstream Vera Sans Mono',monospace;font-size:0.95em;line-height:15.960000038147px;padding:0.5em;border:1px solid rgb(204,204,204);color:rgb(0,0,0);background-color:rgb(248,248,248)"><span style="color:rgb(96,160,176);font-style:italic">} // namespace llvm</span></pre></div></blockquote>used in this patch.</div><div><br></div><div>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?</div></div></blockquote></span><div><br>I think that'd be the best idea, yes (revert, discuss, then commit whatever ends up being decided on).<br></div></div></div></div></blockquote><div><br></div></span><div>Reverted this patch in r240353. Will revert the other two patches separately.</div></div></div></div></blockquote><div><br></div>The other two reverted in r240390 and r240393.<br><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Also, what the preferred solution here would be?<br></div></div></blockquote></span><div><br>Probably part of the discussion we can have on llvm-dev about this.<br> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div>  1. leave the comments (or the lack thereof) alone, don't even fix incorrect or inconsistent ones<br></div><div>  2. only fix incorrect comments</div><div>  3. fix incorrect comments and make all existing namespace ending comments consistent</div></div></blockquote></span><div><br>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.<br> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>  4. 3. + add namespace ending comments to all namespaces spanning more than X lines</div><div>  5. something else?</div><div><br></div><div>In cases 2-4 we need to decide which format of namespace ending comments to use:</div><div>  a. // llvm</div><div>  b. // namespace llvm</div></div></blockquote></span><div><br>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.<br><br>I guess if I were picking the comment out of thin air I might say "llvm namespace".<br> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>  c. // end namespace llvm</div><div>  d. // end of namespace llvm</div><div>  e. // end llvm namespace</div><div>  f. something else</div><div><br></div><div>This patch corresponds to 4b.</div></div>
</blockquote></span></div><br></div></div>
</blockquote></span></div><br>
</div></div>
</blockquote></div><br>
</div></div>