<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 12, 2014 at 11:26 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank" class="cremed">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Wed, Nov 12, 2014 at 1:46 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><div dir="ltr"><div style="font-family:arial,sans-serif;font-size:13px">Grml... Really prefer reviews on phabricator ...</div></div></span></blockquote><div><br></div></span><div>(I try to set up the command-line tool every now and then, it always ends up being a huge mess. And using the web interface takes 10x as long as just emailing the output of `svn diff` – I need to figure out how to convince svn how to include more context, click through a a bunch of web forms, etc. Since it's a small patch, I figured it's ok. If uploading to phab was easier, I'd use it more.)</div></div></div></div></blockquote><div><br></div><div>I agree on your points and also want this to be easier.</div><div><br></div><div>For me as a reviewer it's exactly the opposite. Instead of just clicking and leaving comments, I need to open the attached patch and copy&paste stuff I want to leave comments on. If I need to have context, I need to apply it locally.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><div dir="ltr"><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">>+    } else if (Limit != 0 && Line.First->isNot(tok::kw_namespace) &&</div><div style="font-family:arial,sans-serif;font-size:13px">>+               (Line.First->isNot(tok::kw_extern) ||</div><div style="font-family:arial,sans-serif;font-size:13px">>+                (Line.First->Next && !Line.First->Next->isStringLiteral()))) {</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Isn't this doing something entirely different, namely not contracting such declaration to a single line. I'd be fine with that, but it should be called out explicitly.. And fixes part of <a href="http://llvm.org/PR21419" target="_blank" class="cremed">llvm.org/PR21419</a>.</div></div></span></blockquote><div><br></div></span><div>Ah, true. I only did what my description claimed at first, and then my testcase failed since it got formatted as a single line, so I figured this should probably be handled the same way namespaces are too. I'll call it out in the commit message.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><div dir="ltr"><div style="font-family:arial,sans-serif;font-size:13px">Also, this would prevent:<br></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">  extern "C" int f() { return 42; }</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Right? Probably not what we want.</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px"><div>>+        PreviousLine->First->isNot(tok::kw_namespace) &&</div><div>>+        PreviousLine->First->isNot(tok::kw_extern))</div></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">!PreviousLine->First->isOneOf(tok::kw_namespace, tok::kw_extern)</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Also, this prevents removing the empty line in</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">  extern "C" int f() {</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">    int i = 42;</div><div style="font-family:arial,sans-serif;font-size:13px">    return i;</div><div style="font-family:arial,sans-serif;font-size:13px">  }</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">Also not what we want, I guess.</div></div></span></blockquote><div><br></div></span><div>Great catch! Updated patch attached.</div></div><br></div></div>
</blockquote></div><br></div></div>