[patch] Ignore KeepEmptyLinesAtTheStartOfBlocks for extern "C" blocks

[Daniel Jasper]

On Wed, Nov 12, 2014 at 11:26 PM, Nico Weber <thakis at chromium.org> wrote:

> On Wed, Nov 12, 2014 at 1:46 PM, Daniel Jasper <djasper at google.com> wrote:
>
>> Grml... Really prefer reviews on phabricator ...
>>
>
> (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.)
>

I agree on your points and also want this to be easier.

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.


>> >+    } else if (Limit != 0 && Line.First->isNot(tok::kw_namespace) &&
>> >+               (Line.First->isNot(tok::kw_extern) ||
>> >+                (Line.First->Next &&
>> !Line.First->Next->isStringLiteral()))) {
>>
>> 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 llvm.org/PR21419.
>>
>
> 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.
>
>
>> Also, this would prevent:
>>
>>   extern "C" int f() { return 42; }
>>
>> Right? Probably not what we want.
>>
>> >+        PreviousLine->First->isNot(tok::kw_namespace) &&
>> >+        PreviousLine->First->isNot(tok::kw_extern))
>>
>> !PreviousLine->First->isOneOf(tok::kw_namespace, tok::kw_extern)
>>
>> Also, this prevents removing the empty line in
>>
>>   extern "C" int f() {
>>
>>     int i = 42;
>>     return i;
>>   }
>>
>> Also not what we want, I guess.
>>
>
> Great catch! Updated patch attached.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141113/9e3cdeba/attachment.html>