[PATCH] D88239: [clang-format] Fix spaces around */& in multi-variable declarations

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Sorry for being a bit late here and thanks @klimek for bringing this to my 
attention.
This has been years ago, but if I reconstruct my thinking (and look at the test 
cases), then I'd say that "left" alignment should not be applied to 
multi-variable decl statements ever.

  int* a, b;

Just looks too much like it's declaring two int pointers when it is not. And it 
leads to all sorts of weirdness when doing line wrapping.
So, I'd really like to keep the current behavior.
If you'd want to change it because it makes a difference for someone's 
codebase, I'd almost lean towards introducing an additional 
PointerAlignmentStyle PAS_LeftEvenForMutliVarDeclStatements :).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88239/new/

https://reviews.llvm.org/D88239

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61663: [clang-format] Fix a JavaScript import order bug.

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Generally, upload patches with the full file as context (that will prevent 
Phabricator's "Context not available")
But this change looks good. Thank you.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61663/new/

https://reviews.llvm.org/D61663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60558: [clang-format] Fix indent of trailing raw string param after newline

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good. Thank you.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60558/new/

https://reviews.llvm.org/D60558



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53072: [clang-format] Introduce the flag which allows not to shrink lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Ok, but this behavior is still intended. You are setting clang-format to a 
format where it is breaking after binary operators and then added a break 
before a binary operator. clang-format assumes that this is not intended and 
that you will want this cleaned up.

E.g.:

  $ cat /tmp/format.cc
  if ( a
&& b) {
  }
  
  $ clang-format -style="{ColumnLimit: 0, BreakBeforeBinaryOperators: All}" 
/tmp/format.cc
  if (a
  && b) {
  }
  
  $ clang-format -style="{ColumnLimit: 0}" /tmp/format.cc
  if (a && b) {
  }

I believe that this behavior is right and we should not work around it with an 
additional flag.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53072/new/

https://reviews.llvm.org/D53072



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Look at getGoogleStyle(). It has a bunch of language-specific configs at the 
bottom. You can do the same for TableGen in getLLVMStyle().


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55964/new/

https://reviews.llvm.org/D55964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

This seem to conceptually be a list of things rather than an array subscript, 
though, right?  Could we alternatively set SpacesInContainerLiterals to false 
for LK_TableGen?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55964/new/

https://reviews.llvm.org/D55964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53072: [clang-format] Introduce the flag which allows not to shrink lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Without understanding this in more detail I do not know how to move forward 
with this. What this patch is describing is what should already be the case 
with ColumnLimit set to zero. If it isn't this might be a bug or there might be 
a different way to move forward. However, the flag as is does not make sense to 
me without more information.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53072/new/

https://reviews.llvm.org/D53072



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53072: [clang-format] Introduce the flag which allows not to shrink lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

  $ cat /tmp/test.cc
  int foo(int a,
 int b) {
f();
}
  
  $ clang-format -style="{ColumnLimit: 0}" /tmp/test.cc
  int foo(int a,
  int b) {
f();
  }

Is this not what you want? If so, in what way?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53072/new/

https://reviews.llvm.org/D53072



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53072: [clang-format] Introduce the flag which allows not to shrink lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I don't quite understand. What you are describing should already be the 
behavior with ColumnLimit=0 and I think your test should pass without the new 
option. Doesn't it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53072/new/

https://reviews.llvm.org/D53072



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54795: [clang-format] Do not treat asm clobber [ as ObjCExpr, refined

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Does this also work for _asm and __asm?


Repository:
  rC Clang

https://reviews.llvm.org/D54795



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I think this roughly looks fine. Krasimir, any thoughts?




Comment at: unittests/Format/FormatTest.cpp:11854
+  // case above.
+  {
+auto Style = getGoogleStyle();

No need to use a scope here. Feel free to redefine Style.

If in fact you feel like that is getting out of hand, maybe extract a separate 
TEST_F() for this.



Comment at: unittests/Format/FormatTest.cpp:11865
+  }
+  {
+verifyFormat("SomeFunction(\n"

No need for this scope.


Repository:
  rC Clang

https://reviews.llvm.org/D52676



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11604
+   "  x.end(),   //\n"
+   "  [&](int, int) { return 1; });\n"
"}\n");

oleg.smolsky wrote:
> krasimir wrote:
> > This looks a bit suspicious: I'd expect a break before the first arg to be 
> > forced only when there exists a multiline (after formatting) lambda 
> > expression arg. Is this (multiline vs. lambdas fitting 1 line) something 
> > that we (can) differentiate with respect to? djasper@ might have an insight 
> > on this.
> Well, yes, I can see where you are coming from - the lambda is short and 
> would fit. Unfortunately, I am not sure how to implement this nuance... I 
> think I'd need to get the length of the unwrapped line and then check whether 
> it fits in TokenUnnotator.cc
> 
> Also, I personally favor less indentation (i.e. full width for the lambda) as 
> that prevents drastic reformat when the lambda body changes. (that's why this 
> patch exists)
I agree with Krasimir here.

If you prefer less indentation, great. Set AlignAfterOpenBracket to 
"AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested 
before).

In more seriousness, I think getting all these cases right, I appreciate that 
it is difficult. However, I also like to make sure that we do get them right. 
Changing clang-format's behavior for any of these cases is not a small thing, 
it will cause churn for *a lot* of people. We should try really hard to not 
have changes in there that people will find detrimental. This of course is 
subjective, so we won't get to 100%, but if in doubt for specific cases, let's 
err on the side of not changing the current behavior.



Comment at: unittests/Format/FormatTest.cpp:11736
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"

oleg.smolsky wrote:
> krasimir wrote:
> > Could we please have a test case where there are several args packed on the 
> > first line, then a line break, then an arg, then a multiline lambda as a 
> > last arg (illustrating that we don't pull the first arg down if there's 
> > only a multiline lambda as the last arg):
> > ```
> > function(a, b, ccc,
> >  d, [] () {
> >   body
> > });
> > ```
> Sure, that seems to work, but not in the way you expected :) I'll update the 
> patch...
> 
> ```
>   verifyFormat("function(a, b, c, //\n"
>" d, [this, that] {\n"
>"   //\n"
>" });\n");
> ```
We should try to prevent that (unless it's also the current behavior of 
course). People have filed various bugs about this before and it is not 
generally an accepted formatting.


Repository:
  rC Clang

https://reviews.llvm.org/D52676



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.



In https://reviews.llvm.org/D52676#1268748, @oleg.smolsky wrote:

> In https://reviews.llvm.org/D52676#1268706, @djasper wrote:
>
> > Ok, I think I agree with both of you to a certain extent, but I also think 
> > this change as is, is not yet right.
> >
> > First, it does too much. The original very first example in this CL is 
> > actually not produced by clang-format (or if it is, I don't know with which 
> > flag combination). It is a case where the lambda is the last parameter.
>
>
> Right, I cheated and created that example by hand. My apologies for the 
> confusion. I've just pasted real code that I pumped through `clang-format`. 
> Please take a look at the updated summary.
>
> > Second, I agree that the original behavior is inconsistent in a way that we 
> > have a special cases for when the lambda is the very first parameter, but 
> > somehow seem be forgetting about that when it's not the first parameter. 
> > I'd be ok with "fixing" that (it's not a clear-cut bug, but I think the 
> > alternative behavior would be cleaner). However, I don't think your patch 
> > is doing enough there. I think this should be irrespective of bin-packing 
> > (it's related but not quite the same thing),
>
> Also there is a special case for multiple lambdas. It forces line breaks. 
> That aside, for the single-lambda case, are you suggesting that it is always 
> "pulled up", irrespective of its place? That would contradict the direction I 
> am trying to take as I like `BinPackArguments: false` and so long lamba args 
> go to their own lines. This looks very much in line with what bin packing is, 
> but not exactly the same. Obviously, I can add a new option `favor line 
> breaks around multi-line lambda`.


I don't think I am. You are right, there is the special case for multi-lambda 
functions and I think we should have almost the same for single-lambda 
functions. So, I think I agree with you and am in favor of:

  someFunction(
  a,
  [] {
// ...
  },
  b);

And this is irrespective of BinPacking. I think this is always better and more 
consistent with what we'd be doing if "a" was not there. The only caveat is 
that I think with BinPacking true or false, we should keep the more compact 
formatting if "b" isn't there and the lambda introducer fits entirely on the 
first line:

  someFunction(a, [] {
// ...
  });

> Could you look at the updated summary (high level) and the new tests I added 
> (low level) please? Every other test passes, so we have almost the entire 
> spec. I can add a few cases where an existing snippet is reformatted with 
> `BinPackArguments: false` too.

Not sure I am seeing new test cases and I think at least a few cases are 
missing from the entire spec, e.g. the case above.

Also, try to reduce the test cases a bit more:

- They don't need the surrounding functions
- You can force the lambda to be multi-line with a "//" comment
- There is no reason to have the lambda be an argument to a member function, a 
free-standing function works just as well

This might seem nit-picky, but in my experience, the more we can reduce the 
test cases, the easier to read and the less brittle they become.

>> ...and it should also apply to multiline strings if 
>> AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in 
>> the same patch, but we should have a plan of what we want the end result to 
>> look like and should be willing to get there.
>> 
>> Maybe to start with, we need the complete test matrix so that we are 
>> definitely on the same page as to what we are trying to do. I imagine, we 
>> would need tests for a function with three parameters, where two of the 
>> parameters are short and one is a multiline lambda or a multiline string 
>> (and have the lambda be the first, second and third parameter). Then we 
>> might need those for both bin-packing and no-bin-packing configurations.




Repository:
  rC Clang

https://reviews.llvm.org/D52676



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Ok, I think I agree with both of you to a certain extent, but I also think this 
change as is, is not yet right.

First, it does too much. The original very first example in this CL is actually 
not produced by clang-format (or if it is, I don't know with which flag 
combination). It is a case where the lambda is the last parameter. For me, it 
actually produces:

  void f() {
something.something.something.Method(some_arg, [] {
  // the code here incurs
  // excessive wrapping
  // such as
  Method(some_med_arg, some_med_arg);
  some_var = some_expr + something;
});
  }

And that's an intentional optimization for a very common lambda use case. It 
reduces indentation even further and makes some coding patterns much nicer. I 
think (but haven't reproduced) that you patch might change the behavior there.

Second, I agree that the original behavior is inconsistent in a way that we 
have a special cases for when the lambda is the very first parameter, but 
somehow seem be forgetting about that when it's not the first parameter. I'd be 
ok with "fixing" that (it's not a clear-cut bug, but I think the alternative 
behavior would be cleaner). However, I don't think your patch is doing enough 
there. I think this should be irrespective of bin-packing (it's related but not 
quite the same thing), and it should also apply to multiline strings if 
AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in 
the same patch, but we should have a plan of what we want the end result to 
look like and should be willing to get there.

Maybe to start with, we need the complete test matrix so that we are definitely 
on the same page as to what we are trying to do. I imagine, we would need tests 
for a function with three parameters, where two of the parameters are short and 
one is a multiline lambda or a multiline string (and have the lambda be the 
first, second and third parameter). Then we might need those for both 
bin-packing and no-bin-packing configurations.


Repository:
  rC Clang

https://reviews.llvm.org/D52676



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50132: [clang-format] Add some text proto functions to Google style

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D50132



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50078: clang-format: support aligned nested conditionals formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I don't have very strong opinions here and I agree that we will need to improve 
the conditional formatting, especially as this kind of ternary-chaining is 
becoming more popular because of constexpr.

My personal opinion(s):

- I think this is a no-brainer for BreakBeforeTernaryOperators=false
- I'd be ok with the suggestion for BreakBeforeTernaryOperators=true
- I don't think the alignment of "?" and ":" (in the WhitespaceManager) should 
be always on. I think we'd need a flag for that part

Manuel, Krasimir, WDYT?


Repository:
  rC Clang

https://reviews.llvm.org/D50078



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49580: [clang-format] Adding style option for absolute formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Ok, so IIUC, understanding that @end effective ends a section much like "}" 
would address the currently observed problems?


https://reviews.llvm.org/D49580



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49580: [clang-format] Adding style option for absolute formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In my opinion, this only addresses one edge case where clang-format -lines 
output is not identical with a full reformatting. I believe they cannot all 
usefully be avoided. As such, I am unsure that this option carries its weight 
of making the implementation more complex.
How often does this happen for you? Why can't you just format the additional 
incorrect line?


https://reviews.llvm.org/D49580



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49580: [clang-format] Adding style option for absolute formatting

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Could you explain what problem this is fixing?


https://reviews.llvm.org/D49580



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: unittests/Format/FormatTest.cpp:3444
+
+  verifyFormat("Constructor()\n"
+   ": (a), b(b) {}",

djasper wrote:
> I find these tests hard to read and reason about. How about writing them like 
> this:
> 
>   for (int i = 0; i < 4; ++i) {  // There might be a better way to iterate
> // Test all combinations of parameters that should not have an effect.
> Style.AllowAllParametersOfDeclarationOnNextLine = i & 1;
> Style.AllowAllConstructorInitializersOnNextLine = i & 2;
> 
> Style.AllowAllConstructorInitializersOnNextLine = true;
> verifyFormat("SomeClassWithALongName::Constructor(\n"
>  "int , int b)\n"
>  ": (a), b(b) {}",
>  Style);
> // ... more tests
> 
> 
> Style.AllowAllConstructorInitializersOnNextLine = false;
> verifyFormat("SomeClassWithALongName::Constructor(\n"
>  "int , int b)\n"
>  ": (a)\n"
>  ", b(b) {}",
>  Style);
> // ... more tests
>   }
Err.. The second line inside the for-loop was meant to read:

  Style.AllowAllArgumentsOnNextLine = i & 2;


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:760
 (!Style.AllowAllParametersOfDeclarationOnNextLine &&
  State.Line->MustBeDeclaration) ||
+(!Style.AllowAllArgumentsOnNextLine &&

russellmcc wrote:
> djasper wrote:
> > This still looks suspicious to me. State.Line->MustBeDeclaration is either 
> > true or false meaning that AllowAllParametersOfDeclarationOnNextLine or 
> > AllowAllArgumentsOnNextLine can always affect the behavior here, leading to 
> > BreakBeforeParameter to be set to true, even if we are in the case for 
> > PreviousIsBreakingCtorInitializerColon being true.
> > 
> > So, my guess would be that if you set one of AllowAllArgumentsOnNextLine 
> > and AllowAllParametersOfDeclarationOnNextLine to false, then 
> > AllowAllConstructorInitializersOnNextLine doesn't have an effect anymore.
> > 
> > Please verify, and if this is true, please fix and add tests. I think this 
> > might be easier to understand if you pulled the one if statement apart.
> Actually, I think this logic works.  The case you are worried about 
> (interaction between arguments, parameters, and constructor initializers) is 
> already tested in the unit tests in the 
> `AllowAllConstructorInitializersOnNextLine` test.  The specific concern you 
> have is solved by the separate if statement below.
> 
> I agree that this logic is a bit complex, but I think it's necessary since in 
> most cases we don't want to change the existing value of 
> `State.Stack.back().BreakBeforeParameter` - we only want to change this when 
> we are sure we should or shouldn't bin-pack.  I've tried hard not to change 
> any existing behavior unless it was clearly a bug.  I think we could simplify 
> this logic if we wanted to be less conservative.
> 
> I'm not sure what you mean by breaking up the if statement - did you mean 
> something like this?  To me, this reads much more verbose and is a bit more 
> confusing; however I'm happy to edit the diff if it makes more sense to you:
> 
> ```
> // If we are breaking after '(', '{', '<', or this is the break after a 
> ':'
> // to start a member initializater list in a constructor, this should not
> // be considered bin packing unless the relevant AllowAll option is false 
> or
> // this is a dict/object literal.
> bool PreviousIsBreakingCtorInitializerColon =
> Previous.is(TT_CtorInitializerColon) &&
> Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon;
> 
> if (!(Previous.isOneOf(tok::l_paren, tok::l_brace, TT_BinaryOperator) ||
>   PreviousIsBreakingCtorInitializerColon))
>   State.Stack.back().BreakBeforeParameter = true;
> 
> if (!Style.AllowAllParametersOfDeclarationOnNextLine &&
> State.Line->MustBeDeclaration)
>   State.Stack.back().BreakBeforeParameter = true;
> 
> if (!Style.AllowAllArgumentsOnNextLine && !State.Line->MustBeDeclaration)
>   State.Stack.back().BreakBeforeParameter = true;
> 
> if (!Style.AllowAllConstructorInitializersOnNextLine &&
> PreviousIsBreakingCtorInitializerColon)
>   State.Stack.back().BreakBeforeParameter = true;
> 
> if (Previous.is(TT_DictLiteral)))
>   State.Stack.back().BreakBeforeParameter = true;
> 
> // If we are breaking after a ':' to start a member initializer list,
> // and we allow all arguments on the next line, we should not break
> // before the next parameter.
> if (PreviousIsBreakingCtorInitializerColon &&
> Style.AllowAllConstructorInitializersOnNextLine)
>   State.Stack.back().BreakBeforeParameter = false;
> ```
I find it hard to say whether you actually have a test for this. I'll make a 
suggestion on how to make these tests more maintainable below.

I understand now how this works, but the specific case I was worried about is:

  AllowAllConstructorInitializersOnNextLine = true
  AllowAllArgumentsOnNextLine = false
  AllowAllParametersOfDeclarationOnNextLine = false

(likely you only need one of the latter, but I am not sure which one :) )

This works, because you are overwriting the value again in the subsequent if 
statement (which I hadn't seen).

However, I do personally find that logic hard to reason about (if you have a 
sequence of if statements where some of them might overwrite the same value).

Fundamentally, you are doing:

  if (something)
value = true;

  if (otherthing)
value = false;

I think we don't care about (don't want to change) a pre-existing "value = 
true" and so we actually just need:

  if (something && !otherthing)
value = true;

Or am I missing something? If not, let's actually use the latter and simplify 
the "something && !otherthing" (e.g. by pulling out variables/functions) until 
it is readable again. Let me know if you want me to take a stab at that (I 
promise it won't take weeks again :( ).



Comment at: unittests/Format/FormatTest.cpp:3444
+
+  

[PATCH] D48363: [clang-format] Enable text proto formatting in common functions

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D48363



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

You are right that this behavior is what the code authors, but also many other 
people, like to have and so it is what is engrained in clang-format. There are 
likely about a million things that fall into the same category. Now we might 
find that the current default is actually wrong and we have changed many 
defaults in the past. I don't believe that's the case here and there are more 
opinions on this thread.

If people still disagree, there is the question of whether a flag should be 
introduced, and for every single flag that was introduced, somebody did not 
agree with what clang-format was doing. We have established the bar that has to 
be met in order for the cost-benefit-ratio of options to be beneficial. I don't 
see any reason to make an exception here. The fact that it is a rare corner 
case makes it less likely to be important enough to carry the weight of an 
option. Creating a second option class (nested, hidden, ...) does not seem to 
improve the situation to me. That will just create a subsequent mass of 
additional flags that people have wanted in the past or will want in the future 
that don't actually carry their weight (in my opinion - and likely speaking for 
most other clang-format contributors). Discoverability is only one of the 
concerns about the costs of flags.

So in short, unless you can actually meet the usual requirements for flags, the 
answer is: no.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good. Sorry for the delay.


Repository:
  rC Clang

https://reviews.llvm.org/D43015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

The normal rule for formatting options apply. If you can dig up a public style 
guide and a project of reasonable size where it is used, we can add an option.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:760
 (!Style.AllowAllParametersOfDeclarationOnNextLine &&
  State.Line->MustBeDeclaration) ||
+(!Style.AllowAllArgumentsOnNextLine &&

This still looks suspicious to me. State.Line->MustBeDeclaration is either true 
or false meaning that AllowAllParametersOfDeclarationOnNextLine or 
AllowAllArgumentsOnNextLine can always affect the behavior here, leading to 
BreakBeforeParameter to be set to true, even if we are in the case for 
PreviousIsBreakingCtorInitializerColon being true.

So, my guess would be that if you set one of AllowAllArgumentsOnNextLine and 
AllowAllParametersOfDeclarationOnNextLine to false, then 
AllowAllConstructorInitializersOnNextLine doesn't have an effect anymore.

Please verify, and if this is true, please fix and add tests. I think this 
might be easier to understand if you pulled the one if statement apart.


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46519: [clang-format] Respect BreakBeforeClosingBrace while calculating length

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Generally looks good.




Comment at: lib/Format/ContinuationIndenter.cpp:93
+  break;
+if (End->Next->is(tok::r_brace)) {
+  const ParenState *State = FindParenState(End->Next->MatchingParen);

I r_brace enough? Do we have something similar for TT_ArrayInitializer? I'd 
look at usages of BreakBeforeClosingBrace to determine.


Repository:
  rC Clang

https://reviews.llvm.org/D46519



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46519: [clang-format] Respect BreakBeforeClosingBrace while calculating length

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:44
+  int MatchingStackIndex = Stack.size() - 1;
+  while (MatchingStackIndex >= 0 && Stack[MatchingStackIndex].Tok != )
+--MatchingStackIndex;

I think this needs a long explanatory comment, possibly with an example of the 
problem it is actually solving.

Also, I am somewhat skeptical as we are using this logic for all paren kinds, 
not just braces. That seems to be unnecessary work and also might be unexpected 
at some point (although I cannot come up with a test case where that would be 
wrong).



Comment at: lib/Format/ContinuationIndenter.cpp:49
+  break;
+while (MatchingStackIndex >= 0 &&
+   Stack[MatchingStackIndex].Tok != End->Next->MatchingParen)

Maybe pull out a lambda:

  auto FindParenState = [&](const FormatToken *Tok) {
while (MatchingStackIndex >= 0 && Stack[MatchingStackIndex].Tok != )
  --MatchingStackIndex;
return MatchingStackIndex >= 0 ? [MatchingStackIndex] : nullptr;
  };

Then you could write the rest as:

  ...
  FindParenState();
  const auto* End = Tok.MatchingParen
  for (; End->Next; End = End->Next) {
if (End->Next->CanBreakBefore || !End->Next->closesScope())
  break;
auto ParenState = FindParenState(End->Next->MatchingParen);
if (ParenState && ParenState.BreakBeforeClosingBrace)
  break;
  }
  return End->TotalLength - Tok.TotalLength + 1;


(not entirely sure it's better)



Comment at: lib/Format/ContinuationIndenter.h:215
+  /// \brief The token opening this parenthesis level, or nullptr if this level
+  /// is opened by fake parenthesis.
+  const FormatToken *Tok;

The comment should include that this is not considered for memoization as the 
same state will always be represented by the same token (similar to other 
below). I wonder whether we should actually move the fields that don't affect 
memoization out to a different structure at some point.


Repository:
  rC Clang

https://reviews.llvm.org/D46519



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46143: [clang-format/ObjC] Use getIdentifierInfo() instead of tok::identifier

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good, thank you.


Repository:
  rC Clang

https://reviews.llvm.org/D46143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: include/clang/Format/Format.h:154
 
+  /// \brief If a function call, initializer list, or template
+  /// argument list doesn't fit on a line, allow putting all

writing just "initializer list" is confusing, especially next to the 
constructor initializer list below. Maybe "brace initializer list"?

Also, if this influences initializer lists and template argument lists, please 
add tests for those.

If you change this file, please run docs/tools/dump_format_style.py to update 
the docs.

(also, why is this comment so narrow?)



Comment at: include/clang/Format/Format.h:171
+
+  /// \brief If a constructor initializer list doesn't fit on a line, allow
+  /// putting all initializers onto the next line, if

I think this comment is a bit confusing. The "initializer list" does fit on one 
line.



Comment at: lib/Format/ContinuationIndenter.cpp:757
+Previous.is(TT_DictLiteral) ||
+(!Style.AllowAllArgumentsOnNextLine && !State.Line->MustBeDeclaration))
   State.Stack.back().BreakBeforeParameter = true;

nitpick: move this up one line so it's next to the case for 
AllowAllParametersOfDeclarationOnNextLine.



Comment at: unittests/Format/FormatTest.cpp:3438
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ColumnLimit = 60;

Only testing this for BCIS_BeforeComma seems a bit bad to me. Is there a reason 
for it?

Also, I think we should have a test where the constructor declaration itself 
does not fit on one line, e.g. what's the behavior for:

  Constructor(int param1,
  ...
  int paramN) {
  : aa(a), b(b) { .. }


https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45373: [clang-format] Don't remove empty lines before namespace endings

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.




Comment at: lib/Format/NamespaceEndCommentsFixer.h:24
 
+// Finds the namespace token corresponding to a closing namespace `}`, if that
+// is to be formatted.

I don't understand the "if that is to be formatted part". What do you mean?


Repository:
  rC Clang

https://reviews.llvm.org/D45373



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45526: [clang-format] Prefer breaking after ObjC category close paren

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.




Comment at: lib/Format/TokenAnnotator.cpp:2276
+  // In Objective-C type declarations, prefer breaking after the category's
+  // close paren instead of after the open paren.
+  if (Line.Type == LT_ObjCDecl && Left.is(tok::l_paren) && Left.Previous &&

This is still using the old wording.


Repository:
  rC Clang

https://reviews.llvm.org/D45526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45526: [clang-format] Prefer breaking after ObjC category close paren

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I understand that, but the test example does not break after the closing paren. 
It breaks after the subsequent "<".


Repository:
  rC Clang

https://reviews.llvm.org/D45526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45526: [clang-format] Prefer breaking after ObjC category close paren

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Both patch and comment inside patch say "break after closing paren" and yet 
that's not what the test or example in the description actually do. Why is that?


Repository:
  rC Clang

https://reviews.llvm.org/D45526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45004: [clang-format] Always indent wrapped Objective-C selector names

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added inline comments.
This revision is now accepted and ready to land.



Comment at: unittests/Format/FormatTest.cpp:7666
   FormatStyle Style = getLLVMStyle();
+  // ObjC ignores IndentWrappedFunctionNames when wrapping methods.
   Style.IndentWrappedFunctionNames = false;

maybe: .. and always indents. ?


Repository:
  rC Clang

https://reviews.llvm.org/D45004



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45521: [clang-format] Improve ObjC guessing heuristic by supporting all @keywords

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D45521



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45498: [clang-format] Don't insert space between ObjC class and lightweight generic

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.




Comment at: lib/Format/TokenAnnotator.cpp:2357
+  return false;
+else
+  // Protocol list -> space if configured. @interface Foo : Bar 

Don't use else after return.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

I'd have a slight preference for writing:

  bool IsLightweightGeneric = 
  Right.MatchingParen && Right.MatchingParen->Next &&
  Right.MatchingParen->Next->is(tok::colon);
  return !IsLightweightGeneric && Style.ObjCSpaceBeforeProtocolList;

Then I think it might not even need a comment (or a shorter one).


Repository:
  rC Clang

https://reviews.llvm.org/D45498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Ok, you know the ObjC community much better than I do. If you think that adding 
the indentation for ObjC irrespective of the option works for most people, I am 
happy to go with it.


Repository:
  rC Clang

https://reviews.llvm.org/D45004



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Please read:

  
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options

In this case in particular, I would be very interested in a style guide that 
defines how Allman brace style and lambdas work together. IMO, it has so many 
corner cases that it mostly doesn't work there. If we find a good and 
consistent way to do this (and your proposal to always wrap before the [] isn't 
a bad idea there), then I think we should make that the default behavior for 
lambdas in Allman style.


Repository:
  rC Clang

https://reviews.llvm.org/D44609



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I'd go to great lengths to avoid adding new config options and so I don't think 
this would be a bad trade-off to make.

Also, note that you might not actually have to change much. A FormatStyle 
already contains a reference to the FormatStyleSet it was created from and you 
can get the FormatStyle for a different language through that. It might just be 
a bit hacky and not easy to understand as is, but maybe there are easy 
abstractions that we could build around it.

At any rate, your proposal (always indenting for ObjC code) is also fine by me. 
Do you think that survey Google-style ObjC code is enough? What does Xcode do 
by default?


Repository:
  rC Clang

https://reviews.llvm.org/D45004



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good, thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D45185



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44996: [clang-format] Ensure ObjC selectors with 0 args are annotated correctly

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:1347
+} else if (Current.isOneOf(tok::identifier, tok::kw_new) &&
+   Current.Previous && Current.Previous->is(TT_CastRParen) &&
+   Current.Previous->MatchingParen &&

benhamilton wrote:
> benhamilton wrote:
> > djasper wrote:
> > > Isn't it wrong that we detect this as a cast r_paren in the first place?
> > Fantastic question, I asked myself the same thing.
> > 
> > I tried a few variations on this (leaving it as `TT_Unknown`, making a new 
> > type, etc.) and discovered there is at least one existing place which 
> > relies on the `TT_CastRParen` type as an indicator of ObjC code. Example:
> > 
> > https://github.com/llvm-mirror/clang/blob/e37a191e99773959118155304ec2ed0bc0d591c2/lib/Format/TokenAnnotator.cpp#L394
> > 
> > I can fix those, but if I do so, I think it should be a separate diff. What 
> > do you think?
> @djasper and I talked about this on Friday and agreed we should follow up 
> separately.
> 
> I filed https://bugs.llvm.org/show_bug.cgi?id=36976 to follow up.
You might also want to add a

  // FIXME: ...


Repository:
  rC Clang

https://reviews.llvm.org/D44996



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I still really believe that these config options do no make sense and are 
actively confusing.

I see two options:

- We leave this as is
- We fix this right

The right fix here is (IMO) that a style already is per language and thus 
already has a member specifying the language. What we have done in the scope of 
formatting the contents of raw string literals is that you can, in the same 
configuration file/setting, have different styles based on the language being 
formatted. Thus, if we encounter a text-formatted proto inside a C++ raw string 
literal, we switch to the style flags for proto rather than using those for 
C++. You have these different options in the same style configuration file, in 
a different section per language.

So, if you look at a config file, you could see how a user sets the existing 
IndentWrappedFunctionNames to true for ObjC and to false for C++. Now IMO, that 
should mean that ObjC function names are indented and C++ functions are not, 
even if the language of the *file* is ObjC. It doesn't require us to repeat 
these options for each language in the style for each language.

Getting this right will require some refactoring of how a style is passed 
around and used, but I think it'd be the right thing to do.

Look at it the other way. If we go forward with this patch you can have style 
configuration files saying:

  ---

  BasedOnStyle: LLVM
  ---
  Language: Cpp
  IndentWrappedObjCMethodNames: Never
  IndentWrappedFunctionNames: true
  ---
  Language: ObjC
  IndentWrappedObjCMethodNames: Always
  IndentWrappedFunctionNames: false
  ---

I find it very hard to explain what that even means.


Repository:
  rC Clang

https://reviews.llvm.org/D45004



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45185: [clang-format] Support lightweight Objective-C generics

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:2135
+nextToken();
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;

The UnwrappedLineParser is very much about error recovery. Implemented like 
this, it will consume the rest of the file if someone forgets to add the 
closing ">", which can very easily happen when formatting in an editor while 
coding.

Are there things (e.g. semicolons and braces) that clearly point to this having 
happened so that clang-format can recover?



Comment at: lib/Format/UnwrappedLineParser.cpp:2136
+if (FormatTok->Tok.is(tok::less))
+  NumOpenAngles++;
+else if (FormatTok->Tok.is(tok::greater))

I think we generally prefer:

  ++NumOpenAngles;



Comment at: lib/Format/UnwrappedLineParser.cpp:2138
+else if (FormatTok->Tok.is(tok::greater))
+  NumOpenAngles--;
+  } while (!eof() && NumOpenAngles != 0);

I am slightly worried that this loop might be getting more complicated and it 
will be harder to determine that NumOpenAngles cannot be 0 here. Might be worth 
adding an assert. But might be overkill.



Comment at: lib/Format/UnwrappedLineParser.cpp:2181
+  if (FormatTok->Tok.is(tok::less))
+parseObjCLightweightGenericList();
+

Are there more places where we might want to parse a lightweight generic list? 
If this is going to remain they only instance, I think I'd prefer a local 
lambda or just inlining the code. But up to you.



Comment at: lib/Format/UnwrappedLineParser.cpp:2182
+parseObjCLightweightGenericList();
+
   if (FormatTok->Tok.is(tok::colon)) {

nitpick: As the comment refers to this following block as well, I'd remove the 
empty line.


Repository:
  rC Clang

https://reviews.llvm.org/D45185



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45168: [clang-format/ObjC] Do not insert space after opening brace of ObjC dict literal

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D45168



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45169: [clang-format/ObjC] Do not detect "[]" as ObjC method expression

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good. I like option 2 :).


Repository:
  rC Clang

https://reviews.llvm.org/D45169



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D44994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44996: [clang-format] Ensure ObjC selectors with 0 args are annotated correctly

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:1347
+} else if (Current.isOneOf(tok::identifier, tok::kw_new) &&
+   Current.Previous && Current.Previous->is(TT_CastRParen) &&
+   Current.Previous->MatchingParen &&

Isn't it wrong that we detect this as a cast r_paren in the first place?


Repository:
  rC Clang

https://reviews.llvm.org/D44996



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:904
+   : State.Stack.back().Indent);
   if (NextNonComment->LongestObjCSelectorName == 0)
+return MinIndent;

benhamilton wrote:
> djasper wrote:
> > Does this if actually change the behavior in any way? With 
> > LongestObjCSelectorName being 0, isn't that:
> > 
> >   return MinIndent +
> >  std::max(0, ColumnWidth) - ColumnWidth;
> > 
> > (and with ColumnWidth being >= 0, this should be just MinIndent)
> The `- ColumnWidth` part is only for the case where `LongestObjCSelectorName` 
> is *not* 0. If it's 0, we return `MinIndent` which ensures we obey 
> `Style.IndentWrappedFunctionNames`.
> 
> The problem with the code before this diff is when `LongestObjCSelectorName` 
> was 0, we ignored `Style.IndentWrappedFunctionNames` and always returned 
> `State.Stack.back().Indent` regardless of that setting.
> 
> After this diff, when `LongestObjCSelectorName` is 0 (i.e., this is either 
> the first part of the selector or a selector which is not colon-aligned due 
> to block formatting), we change the behavior to indent to at least 
> `State.FirstIndent + Style.ContinuationIndentWidth`, like all other 
> indentation logic in this file.
> 
> I've added some comments explaining what's going on, since this code is quite 
> complex.
I am not saying your change is wrong.  And I might be getting out of practice 
with coding. My question is, what changes if you remove lines 906 and 907 (the 
"if (...) return MinIndent")?


Repository:
  rC Clang

https://reviews.llvm.org/D44994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Well, I disagree. It says: "If you break after the return type of a function 
declaration or definition, do not indent."


Repository:
  rC Clang

https://reviews.llvm.org/D45004



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:899
 if (!State.Stack.back().ObjCSelectorNameFound) {
+  unsigned MinIndent =
+  (Style.IndentWrappedFunctionNames

I think I'd now find this slightly easier to read as:

  unsigned MinIndent = State.Stack.back().Indent;
  if (Style.IndentWrappedFunctionNames)
MinIndent = std::max(MinIndent, State.FirstIndent + 
Style.ContinuationIndentWidth);



Comment at: lib/Format/ContinuationIndenter.cpp:904
+   : State.Stack.back().Indent);
   if (NextNonComment->LongestObjCSelectorName == 0)
+return MinIndent;

Does this if actually change the behavior in any way? With 
LongestObjCSelectorName being 0, isn't that:

  return MinIndent +
 std::max(0, ColumnWidth) - ColumnWidth;

(and with ColumnWidth being >= 0, this should be just MinIndent)


Repository:
  rC Clang

https://reviews.llvm.org/D44994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45004: [clang-format] New style option IndentWrappedObjCMethodNames

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Do we have a public style guide that explicitly says to do this?
That's a requirement for new style options as per 
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.

Also, are we sure that somebody wants the other behavior? Does that make sense 
for ObjC? I'd like to avoid options that nobody actually sets to a different 
value.


Repository:
  rC Clang

https://reviews.llvm.org/D45004



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44816: [clang-format] Do not insert space before closing brace in ObjC dict literal

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Yeah, it's one of these things where neither way would be totally intuitive to 
everyone.


Repository:
  rC Clang

https://reviews.llvm.org/D44816



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44831: [clang-format] Refine ObjC guesser to handle child lines of child lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D44831



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44816: [clang-format] Do not insert space before closing brace in ObjC dict literal

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Generally looks good, one minor simplification.




Comment at: lib/Format/TokenAnnotator.cpp:2484
+  if (Right.is(tok::r_brace) && Right.MatchingParen &&
+  Right.MatchingParen->is(TT_DictLiteral) &&
+  Right.MatchingParen->Previous &&

Could you use Right.MatchingParen->endsSequence(TT_DictLiteral, tok::at) here?


Repository:
  rC Clang

https://reviews.llvm.org/D44816



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44831: [clang-format] Refine ObjC guesser to handle child lines of child lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/Format.cpp:1449
   const AdditionalKeywords ) {
+for (auto Line : AnnotatedLines)
+  if (LineContainsObjCCode(*Line, Keywords))

I would not create a second function here. Just iterate over the tokens here 
and call guessIsObjC recursively with Line->Children. That means we need one 
less for loop overall, I think (I might be missing something).



Comment at: lib/Format/Format.cpp:1455
+
+  static bool LineContainsObjCCode(const AnnotatedLine ,
+   const AdditionalKeywords ) {

Convention would be lineContainsObjCCode.


Repository:
  rC Clang

https://reviews.llvm.org/D44831



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44831: [clang-format] Refine ObjC guesser to handle child lines of child lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/Format.cpp:1542
 };
-for (auto Line : AnnotatedLines) {
-  if (LineContainsObjCCode(*Line))
+llvm::DenseSet LinesToCheckSet;
+LinesToCheckSet.reserve(AnnotatedLines.size());

Wouldn't it be much easier to call this function recursively for Children 
instead of using the lambda as well as this additional set? Lines and their 
children should form a tree, I think, so you should never see the same line 
again during recursion.


Repository:
  rC Clang

https://reviews.llvm.org/D44831



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44790: [clang-format] Fix ObjC style guesser to also iterate over child lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: cfe/trunk/lib/Format/Format.cpp:1544
+return true;
+  for (auto ChildLine : Line->Children) {
+if (LineContainsObjCCode(*ChildLine))

Sorry that I missed this in the original review. But doesn't this still fail 
for Children of Child lines, etc.? I think this probably needs to be fully 
recursive.


Repository:
  rL LLVM

https://reviews.llvm.org/D44790



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44790: [clang-format] Fix ObjC style guesser to also iterate over child lines

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good, thank you!




Comment at: lib/Format/Format.cpp:1517
 
-for (auto  : AnnotatedLines) {
-  for (FormatToken *FormatTok = Line->First; FormatTok;
+auto CheckLineTokens = [](const AnnotatedLine ) {
+  for (const FormatToken *FormatTok = Line.First; FormatTok;

Maybe choose a name that indicates what the bool result value means, e.g. 
LinesContainObjCCode or something.


Repository:
  rC Clang

https://reviews.llvm.org/D44790



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Some last comments, but basically looks good.




Comment at: include/clang/Format/Format.h:352
 
-  /// \brief If ``true``, always break after the ``template<...>`` of a 
template
-  /// declaration.
-  /// \code
-  ///true:  false:
-  ///template   vs. template  class C 
{};
-  ///class C {};
-  /// \endcode
-  bool AlwaysBreakTemplateDeclarations;
+  /// \brief Different ways to break after the template declaration.
+  enum BreakTemplateDeclarationsStyle {

Don't forget to run docs/tools/dump_format_style.py to update the docs.



Comment at: include/clang/Format/Format.h:355
+  /// Do not force break before declaration.
+  /// ``PenaltyBreakTemplateDeclaration`` is taken into account.
+  /// \code

I think it'd be worth having a case here that would actually be formatted 
differently with BTDS_MultiLine.



Comment at: lib/Format/TokenAnnotator.cpp:2248
+  if (Left.ClosesTemplateDeclaration)
+  return Style.PenaltyBreakTemplateDeclaration;
   if (Left.is(TT_ConditionalExpr))

Indentation is off


Repository:
  rC Clang

https://reviews.llvm.org/D42684



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Generally looks good.




Comment at: lib/Format/TokenAnnotator.cpp:2183
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)

Typz wrote:
> djasper wrote:
> > Why is this necessary?
> Otherwise the `PenaltyBreakBeforeFirstCallParameter` is used, which is not 
> consistent with the concept of !Cpp11BraceListStyle (e.g. consider this is an 
> initializer), and gives the following format, which is certainly not the 
> expected result:
> 
>   const std::unordered_map
>   MyHashTable = { { \"a\", 0 },
>   { \"b\", 1 },
>   { \"c\", 2 } };
> 
> (again, the issue only happens when `PenaltyBreakBeforeFirstCallParameter` is 
> increased, e.g. 200 in my case. The default value is 19, so formatting is not 
> affected)
I think PenaltyBreakBeforeFirstCallParameter should not be applied with 
!Cpp11BracedListStyle whenever a brace is found. Cpp11BracedList style means 
that braces are to be treated like function calls, but without it, this doesn't 
make sense. I think this is in some ways better than looking for the "= {".



Comment at: unittests/Format/FormatTest.cpp:6655
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"

Typz wrote:
> djasper wrote:
> > How does this penalty influence the test?
> Because breaking after the opening brace is affected by the 
> `PenaltyBreakBeforeFirstCallParameter` : this is an init braced-list, but it 
> is considered as any function.
> 
> Specifically, my patch needs (see prev. comment) to change the penalty for 
> breaking after the brace, but this applies only when 
> `Cpp11BracedListStyle=false`, as per your earlier comment. So this test just 
> verify that the behavior is indeed not affected.
I see.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D44632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44692: [clang-format] Don't insert space between r_paren and 'new' in ObjC decl

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D44692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Please generally upload diffs with the full file as context so that Phabricator 
can properly expand the context where necessary.

This looks good, though.


Repository:
  rC Clang

https://reviews.llvm.org/D43957



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/Format.cpp:1450
 // Keep this array sorted, since we are binary searching over it.
 static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
 "CGFloat",

benhamilton wrote:
> jolesiak wrote:
> > djasper wrote:
> > > I have concerns about this growing lists of things. Specifically:
> > > - Keeping it sorted is a maintenance concern.
> > > - Doing binary search for basically every identifier in a header seems an 
> > > unnecessary waste.
> > > 
> > > Could we just create a hash set of these?
> > It was a hash set initially: D42135
> > Changed in: D42189
> Happy to do whatever folks recommend; I assume @krasimir's concern in D42189 
> was the startup cost of creating the (read-only) hash set.
> 
> We can automate keeping this sorted with an `arc lint` check, they're quite 
> easy to write:
> 
> https://secure.phabricator.com/book/phabricator/article/arcanist_lint/
Krasimir clarified this to me offline. I have no concerns staying with binary 
search here and for this patch so long as someone builds in an assert that 
warns us when the strings here are not in the right order at some point.


Repository:
  rC Clang

https://reviews.llvm.org/D44632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/Format.cpp:1450
 // Keep this array sorted, since we are binary searching over it.
 static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
 "CGFloat",

I have concerns about this growing lists of things. Specifically:
- Keeping it sorted is a maintenance concern.
- Doing binary search for basically every identifier in a header seems an 
unnecessary waste.

Could we just create a hash set of these?



Comment at: unittests/Format/FormatTest.cpp:12099
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface 
Foo\n@end\n"));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "int DoStuff(CGRect 
rect);\n"));
+  EXPECT_EQ(

jolesiak wrote:
> I know that it's violated in several places in this file (even in two of the 
> three lines above), but I feel like we should keep 80 char limit for column 
> width.
Agreed. Please format this file with clang-format.


Repository:
  rC Clang

https://reviews.llvm.org/D44632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44634: [clang-format] Detect Objective-C for #import

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I'd really like to not parse include/import statements this way. Can you send 
me examples of headers where we fail to recognize them as ObjC and this matters 
(happy for you to send me examples offline).


Repository:
  rC Clang

https://reviews.llvm.org/D44634



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Ok, looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D43902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Nice.. Removed a lot of complexity :). Let's see whether this heuristic is good 
enough.




Comment at: lib/Format/TokenAnnotator.cpp:210
 
-bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
-bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
+bool MightBeFunctionOrObjCBlockType =
+!Contexts[Contexts.size() - 2].IsExpression;

I'd suggest to put a comment here saying that this is for both ObjC blocks and 
Function types, because they look very similar in nature (maybe giving 
examples) and then not actually rename the variables. To me, the long names 
make the code harder to read.

But if you feel strongly the other way, I'd be ok with it.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:329
+  return nullptr;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
+bool IsUsingNamespace =

Can you  make this:

  // C++17 '[[using : foo, bar(baz, blech)]]'

To make clear that we are not looking for kw_namespace here?



Comment at: lib/Format/TokenAnnotator.cpp:332
+AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon);
+if (IsUsingNamespace) {
+  AttrTok = AttrTok->Next->Next->Next;

No braces.



Comment at: lib/Format/TokenAnnotator.cpp:336
+auto parseCpp11Attribute = [](const FormatToken ,
+  bool AllowNamespace) -> const FormatToken * {
+  if (!Tok.isOneOf(tok::identifier, tok::ellipsis))

Do you actually need to put the return type here? I would have thought that it 
can be deduced as you pass back a const FormatToken* from a codepath and 
nullptr from all the others.



Comment at: lib/Format/TokenAnnotator.cpp:342
+return nullptr;
+  if (AllowNamespace &&
+  AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {

No braces.



Comment at: lib/Format/TokenAnnotator.cpp:350
+const FormatToken *ParamToken = AttrTok->Next;
+while (ParamToken && ParamToken->isNot(tok::r_paren))
+  ParamToken = ParamToken->Next;

Sorry that I have missed this before, I thought this was in a different file. 
Can you try to avoid iterating trying to count or match parentheses inside any 
of parseSquare/parseParen/parseAngle. We avoided that AFAICT for everything 
else and I don't think it's necessary here. Especially as you are not actually 
moving the token position forward, it's too easy to create a quadratic 
algorithm here.

Also: Do you actually have a test case for the the parentheses case? This thing 
could use a lot more comments...



Comment at: lib/Format/TokenAnnotator.cpp:366
+  return AttrTok->Next;
+} else {
+  return nullptr;

No braces for single statement ifs. Don't use "else" after "return".



Comment at: lib/Format/TokenAnnotator.cpp:396
+  while (CurrentToken != Cpp11AttributeSpecifierClosingRSquare) {
+if (CurrentToken->is(tok::colon)) {
+  CurrentToken->Type = TT_AttributeColon;

No braces for single-statement ifs.



Comment at: lib/Format/TokenAnnotator.cpp:397
+if (CurrentToken->is(tok::colon)) {
+  CurrentToken->Type = TT_AttributeColon;
+}

What happens if you don't assign this type here? Which formatting decision is 
based on it?



Comment at: unittests/Format/FormatTest.cpp:6064
+  verifyFormat("SomeType s [[unused]] (InitValue);");
+  verifyFormat("SomeType s [[gnu::unused]] (InitValue);");
+  verifyFormat("SomeType s [[using gnu: unused]] (InitValue);");

If this is meant to contrast a TT_AttributeColon from a different colon, that 
doesn't work. "::" is it's own token type coloncolon.


Repository:
  rC Clang

https://reviews.llvm.org/D43902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Right. So the difference is that for blocks, there is always a "(" after the 
"(^.)". That should be easy to parse, no?


Repository:
  rC Clang

https://reviews.llvm.org/D43906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

benhamilton wrote:
> benhamilton wrote:
> > djasper wrote:
> > > benhamilton wrote:
> > > > djasper wrote:
> > > > > This seems suspect. Does it have to be a numeric_constant?
> > > > Probably not, any constexpr would do, I suspect. What's the best way to 
> > > > parse that?
> > > I think this is the same answer for both of your questions. If what you 
> > > are trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be 
> > > enough to look for whether there is a "(" after the ")" or even only 
> > > after "(^)", everything else is already correct IIUC? That would get you 
> > > out of need to parse the specifics here, which will be hard.
> > > 
> > > Or thinking about it another way. Previously, every "(^" would be parsed 
> > > as an ObjC block. There seems to be only a really rare corner case in 
> > > which it isn't (macros). Thus, I'd just try to detect that corner case. 
> > > Instead you are completely inverting the defaults (defaulting to "^" is 
> > > not a block) and then try to exactly parse ObjC where there might be many 
> > > cases and edge cases that you won't even think of now.
> > Hmm. Well, it's not just `FOO(^);` that isn't a block:
> > 
> > ```
> > #define FOO(X) operator X
> > 
> > SomeType FOO(^)(int x, const SomeType& y) { ... }
> > ```
> > 
> > Obviously we can't get this perfect without a pre-processor, but it seems 
> > like our best bet is to only assign mark `TT_ObjCBlockLParen` when we are 
> > sure the syntax is a valid block type or block variable.
> I tried the suggestion to only treat `(^)(` as a block type, but it appears 
> this is the primary place where we set `TT_ObjCBlockLParen`, so I think we 
> really do need to handle the other cases here.
I don't follow your logic. I'd like you to slowly change this as opposed to 
completely going the opposite way.

So currently, the only know real-live problem is "FOO(^);". So address this 
somehow, but still default/error to recognizing too much stuff as a block.

Have you actually seen

  SomeType FOO(^)(int x, const SomeType& y) { ... }

in real code?


Repository:
  rC Clang

https://reviews.llvm.org/D43906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

benhamilton wrote:
> djasper wrote:
> > This seems suspect. Does it have to be a numeric_constant?
> Probably not, any constexpr would do, I suspect. What's the best way to parse 
> that?
I think this is the same answer for both of your questions. If what you are 
trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be enough to 
look for whether there is a "(" after the ")" or even only after "(^)", 
everything else is already correct IIUC? That would get you out of need to 
parse the specifics here, which will be hard.

Or thinking about it another way. Previously, every "(^" would be parsed as an 
ObjC block. There seems to be only a really rare corner case in which it isn't 
(macros). Thus, I'd just try to detect that corner case. Instead you are 
completely inverting the defaults (defaulting to "^" is not a block) and then 
try to exactly parse ObjC where there might be many cases and edge cases that 
you won't even think of now.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Would it be enough to only add the block type case? With the macro false 
positive, there won't be an open paren after the closing paren, right?




Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

This seems suspect. Does it have to be a numeric_constant?



Comment at: unittests/Format/FormatTest.cpp:12029
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));

I am somewhat skeptical about all these tests now being solely about 
guessLanguage. It might be the best choice for some of them, but also, there 
might be other things we do to detect ObjC at some point and then all of these 
tests become meaningless.

Does your change create a different formatting here?


Repository:
  rC Clang

https://reviews.llvm.org/D43906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:288
 
+if (MightBeObjCForRangeLoop) {
+  FormatToken *ForInToken = Left;

There can be only one ObjCForIn token in any for loop, right? If that's the 
case, can we just remember that in addition to (or instead of) 
MightBeObjCForRangeLoop? That way, we wouldn't have to re-iterate over all the 
tokens here, but could just set this directly.


Repository:
  rC Clang

https://reviews.llvm.org/D43904



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:352
+  return false;
+if (!Tok.startsSequence(tok::l_square, tok::l_square))
+  return false;

Just join this if with the previous one.



Comment at: lib/Format/TokenAnnotator.cpp:357
+  return false;
+bool NamespaceAllowed;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'

Replace lines 355-365 with:

  bool IsUsingNamespace = AttrTok && AttrTok->startsSequence( ... );
  if (IsUsingNamespace)
AttrTok = AttrTok->Next->Next->Next;



Comment at: lib/Format/TokenAnnotator.cpp:374
+  return false;
+return AttrTok->startsSequence(tok::r_square, tok::r_square);
+  }

Why not replace these three lines with:

  return AttrTok && AttrTok->startsSequence(..);



Comment at: lib/Format/TokenAnnotator.cpp:400
+  next();
+  return true;
+}

This seems weird. I think if we return true, we should have consumed everything 
up to the closing r_square.



Comment at: unittests/Format/FormatTest.cpp:12083
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));

You are not adding any test (AFAICT) that tests that you have correctly set 
TT_AttributeSpecifier or that you are making any formatting decisions based on 
it. If you can't test it, remove that part of this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D43902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

We have talked about that and none of us agree.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D42787#1025117, @Typz wrote:

> If people don't care about this case, we might as well merge this :-) Just 
> kidding.
>
> The tweak matches both our expectation, the auto-indent behaviour of IDE (not 
> to be trusted, but still probably of 'default' behaviour for many people, 
> esp. when you don't yet use a formatter), and its seems our code base is 
> generally formatted like this. I think it is quite more frequent than 50 
> times in whole LLVM/Clang, but I have no actual numbers; do you have a magic 
> tool to search for such specific "code pattern", or just run clang-format 
> with one option then the next and count the differences ?


I just tweaked a search based on regular expressions. Fundamentally something 
like a line with an open paren and a comma that doesn't end in a comma gives a 
reasonable first approximation. But yes, first formatting with one option, then 
reformatting and looking at numbers of diffs would be interesting. And I would 
bet that even in your codebase diffs are few.

Also double-checked with Richard Smith and he agrees that the current behavior 
is preferable. For comma and plus this doesn't seem overly important, but 
making it:

  aa(b + ccc *
 d);

seems really bad to him as this suggests that we are adding both ccc 
and d.

  aa(b + ccc *
 d);

seems clearer. And for consistency reasons, we should not treat those two cases 
differently.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Got two responses so far.

1. Having to closing braces in the same column is weird, but not that weird. 
Consider



  namespace n {
  void f() {
...
  }
  }

2. Richard Smith (code owner of Clang) says that he doesn't really like the two 
closing braces in the same column, but he always cheats by removing the braces 
for the last case label / default. You never need them. In any case he'd 
strongly prefer the current behavior over adding an extra break and more 
indentation.

In conclusion, I don't think we want to move forward with making clang-format do

  switch (x) {
  case 1:
{
  doSomething();
}
  }

even with IndentCaseLabels: false.


Repository:
  rC Clang

https://reviews.llvm.org/D43183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Are you sure that you are even addressing an important case? I have done some 
research on our codebase and this is something that happens incredibly rarely. 
The reason is that you have to have a very specific combination of line length, 
where the last parameter does not fit on one line if indented back to align 
with the open paren while it does fit on multiple lines if indented right of 
the comma. In all of LLVM/Clang, there seem to be only about 50 cases. How 
certain are you that people actually care?


Repository:
  rC Clang

https://reviews.llvm.org/D42787



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D42684#1022219, @Typz wrote:

> Indeed, seems to apply to classes as well. Maybe I was mislead by my testing, 
> where I did not get the case (possibly because we use 
> `ConstructorInitializerAllOnOneLineOrOnePerLine=true`, so the continuation 
> indenter only sees "short" class declarations unless breaking the template is 
> required because the name is too long).


Not sure how that style flag is related to class declarations, but ok ;).

> So just to be clear, are you saying the whole approach is not the right one, 
> or simply that the "names" of each modes are not?
>  For the name, maybe something like may be better:
> 
> - `Never`
> - `MultiLineDeclaration`, or maybe even `MultiLine`
> - `Always`

Ah, yeah, maybe it's just the name. "AlwaysBreak: Never" and "AlwaysBreak: 
Always" seem not very intuitive to interpret, though. How about just: No, Yes, 
MultiLine?


Repository:
  rC Clang

https://reviews.llvm.org/D42684



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Ah, I thought it was somehow possible to create:

  Constructor(): aa()
   , bb() {},

but I guess clang-format always inserts a break there. Sorry for chasing you in 
circles.


Repository:
  rC Clang

https://reviews.llvm.org/D32525



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: unittests/Format/FormatTest.cpp:8969
+   "barr(1) {}", CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers = 
FormatStyle::BCIS_BeforeComma;
+  verifyFormat("Fooo::Fooo()\n"

This is a useless test if it doesn't actually have multiple initializers 
separated by a comma.



Comment at: unittests/Format/FormatTest.cpp:8971
+  verifyFormat("Fooo::Fooo()\n"
+   ": barr(1) {}", CtorInitializerStyle);
+  CtorInitializerStyle.BreakConstructorInitializers = 
FormatStyle::BCIS_BeforeColon;

Has this been formatted by clang-format? I think it'd break after the comma.


Repository:
  rC Clang

https://reviews.llvm.org/D32525



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

New options for this would not be acceptable IMO. Too much cost for too little 
benefit.

I'd suggest to first make the change to fall back to the style with a regular 
block when there are statements other than break after the closing brace. That 
is always bad, no matter the indentation of the case labels:

  switch (x) {
case 1: {
  doSomething();
}
  doSomethingElse();
  break;
  }

Fixing this is good no matter what.

And then the second question is whether this style should be used or not (with 
IndentCaseLabels: false):

  switch (x) {
  case 1: {
doSomething();
  }
  }

Pro: Saves horizontal and vertical space.
Con: It's weird to have to braces in the same column.

I don't personally have an opinion here, but I'll check with a few LLVM 
developers who work with LLVM style.


Repository:
  rC Clang

https://reviews.llvm.org/D43183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D42684#1022093, @Typz wrote:

> The problem I have is really related to the current 
> `AlwaysBreakTemplateDeclarations` behavior, which does not apply to functions.
>  I set it to false, and I get this:
>
>   template<>
>   void (
>   const bbb & cc);
>   
>
> instead of:
>
>   template<> void 
> (
>   const bbb & cc);
>   
>
> Then when this is fixed the penalty for breaking after the templates part is 
> hardcoded to 10 (`prec::Level::Relational`), which was not always wrapping as 
> expected (that is definitely subjective, but that is the beauty of 
> penalties...)


Ah, I see. However, you are misunderstanding what the parameter is meant to do 
(and I think what the name says). It is controlling whether we "always" break 
before the template declaration (even if everything would fit on just one 
line). Setting it to false, i.e. "not always" breaking, does not imply that 
there is any particular situation in which we need to keep it on the same line.

I understand what you want to achieve, but I don't think it is related to 
whether this is a function or a class declaration, i.e. clang-format also does:

  template 
  class 
  : BB {};

although the template declaration would easily fit on the same line.

So this change does not seem like the right one to make in order to get the 
options to be more intuitive and for you to get the behavior you want. I'll try 
to think about how to achieve that. Do you have any ideas?


Repository:
  rC Clang

https://reviews.llvm.org/D42684



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

If both this and https://reviews.llvm.org/D32525 are submitted, then we also 
need more tests for the combination of the two parameters.




Comment at: include/clang/Format/Format.h:852
+  /// \brief Different ways to break inheritance list.
+  enum BreakInheritanceListStyle {
+/// Break inheritance list before the colon and after the commas.

Update the docs with docs/tools/dump_format_style.py.


Repository:
  rC Clang

https://reviews.llvm.org/D43015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32525: [clang-format] Add SpaceBeforeColon option

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I think this generally looks good, but needs a few more tests.




Comment at: include/clang/Format/Format.h:1204
 
+  /// \brief If ``false``, spaces will be removed before constructor 
initializer
+  /// colon.

When this file is changed, can you also run docs/tools/dump_format_style.py to 
update the docs?



Comment at: unittests/Format/FormatTest.cpp:7539
+  verifyFormat("class Foo : public Bar {};", CtorInitializerStyle);
+  verifyFormat("Foo::Foo(): foo(1) {}", CtorInitializerStyle);
+  verifyFormat("for (auto a : b) {\n}", CtorInitializerStyle);

Can you add tests for the other values of BreakConstructorInitializers 
(BCIS_BeforeColon, BCIS_BeforeComma)?


https://reviews.llvm.org/D32525



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

But you *do* want extra indentation in the case of:

  function(a, 
   b +
   cc);

I understand you argument, but I don't agree at the moment. As is (without 
getting more feedback from others that clang-format is behaving unexpected 
here), I do not want to move forward with this change.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

I think it's possible that this is just a bug/oversight. But I don't fully 
understand the case where it is not behaving as you expect. Can you give me an 
example (config setting + code that's not formatted as you expect)?


Repository:
  rC Clang

https://reviews.llvm.org/D42684



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42729: clang-format: Fix formatting of function body followed by semicolon

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D42729#1022069, @Typz wrote:

> In https://reviews.llvm.org/D42729#994841, @djasper wrote:
>
> > - Of course you find all sorts of errors while testing clang-format on a 
> > large-enough codebase. That doesn't mean that users run into them much.
> > - We have had about 10k clang-format users internally for several years. 
> > The semicolon issue comes up but really rarely and if it does, people 
> > happily fix their code not blaming clang-format.
> >
> >   Unrelated, my point remains that setting BlockKind in TokenAnnotator is 
> > bad enough that I wouldn't want to do it for reaping this small benefit. 
> > And I can't see how you could easily achieve the same thing without doing 
> > that.
>
>
> Just a question though. I there a reason brace matching (and other parts of 
> TokenAnnotations) are not performed before LineUnwrapping? That would 
> probably allow fixing this issue more cleanly (though I am not sure I would 
> have the time to actually perform this probably significant task)...


I think this is just the way it has grown. And brace/paren matching is actually 
done in both places several times by now, I think :(.

Fundamentally, the UnwrappedLineParser had the task of splitting a source file 
into lines and only implemented what it needed for that. The TokenAnnotator 
then did a much more elaborate analysis on each line to determine token types 
and such and distinguish what a "<" actually is (comparison vs. template 
opener). Having these two things be separate makes it slightly easier for error 
recovery and such as the state space they can be in is somewhat more limited.


Repository:
  rC Clang

https://reviews.llvm.org/D42729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2183
   return 0;
+if (Left.Previous && Left.Previous->is(tok::equal) &&
+!Style.Cpp11BracedListStyle)

Why is this necessary?



Comment at: unittests/Format/FormatTest.cpp:6655
+  FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+  AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+  verifyFormat("const std::unordered_map MyHashTable =\n"

How does this penalty influence the test?


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D43290#1020537, @Typz wrote:

> >> Tweaking the penalty handling would still be needed in any case, since the 
> >> problem happens also when Cpp11BracedListStyle is true (though the effect 
> >> is more subtle)
> > 
> > I don't understand this. Which style do you actually care about? With 
> > Cpp11BracedListStyle=true or =false?
>
> I use the `Cpp11BracedListStyle=false` style.
>  However, I think the current behavior is wrong also when 
> `Cpp11BracedListStyle=true`. Here the behavior in this case:
>
> Before :
>
>   const std::unordered_map Something::MyHashTable =
>   {{ "a", 0 },
>{ "b", 1 },
>{ "c", 2 }};
>   
>
> After :
>
>   const std::unordered_set Something::MyUnorderedSet = {
>   { "a", 0 },
>   { "b", 1 },
>   { "c", 2 }};


"Before" is the desired behavior. I don't know whether other people have 
extensively analyzed how to format all the various C++11 braced lists, but we 
have at Google and think this is good. It is formalized here:
https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List_Format

We prefer breaking before "{" at the higher syntactic level or essentially 
before the imaginary function call in place of the braced list.

>>> Generally, I think it is better to just rely on penalties, since it gives a 
>>> way to compare and weight each solution. Then each style can decide what 
>>> should break first: e.g. a style may also have a lower 
>>> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be 
>>> expected...
>> 
>> And with my reasoning, I think exactly the opposite. Penalties are nice, but 
>> if the behavior is generally unwanted, than it's very hard to predict in 
>> which situations it might still occur.
> 
> Yet on the other hand enforcing too much can lead to cases where the 
> optimizer fails to find a solution, and ends up simply not formatting the 
> line: which is fine is the code is already formatted (but if there were the 
> case we would not need the tool at all :-) )
>  The beauty of penalties is that one can tweak the different penalties so 
> that the behavior is "generally" what would be expected: definitely not 
> predictable, but then there are always cases where the "regular" style does 
> not work, and fallback solutions must be used... (for exemple, I would prefer 
> never to never break after an assignment : yet sometimes it is needed...)
> 
> I can change the patch to disallow breaking, but this would be a slightly 
> more risky change, with possibly more side-effects; and i think that would 
> not solve the issue when `Cpp11BracedListStyle=true`.

It is the better call here. I understand that people that set 
Cpp11BracedListStyle to false want this behavior and I think it'll always be a 
surprise when clang-format breaks before the "{". The chance of not coming up 
with a formatting because of this is somewhat slim. It only adds an additional 
two characters (we would also not break before the "="). It'd be really weird 
to only have these exact two line length have a special behavior (slightly 
shorter - everything fits on one line, slightly longer - a split between type 
name and variable is necessary).

There is no issue for Cpp11BracedListStyle=true, the behavior is as desired.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43673: Make module use diagnostics refer to the top-level module

[Daniel Jasper via Phabricator via cfe-commits]

djasper closed this revision.
djasper added a comment.

Submitted as r326023.


https://reviews.llvm.org/D43673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43673: Make module use diagnostics refer to the top-level module

[Daniel Jasper via Phabricator via cfe-commits]

djasper created this revision.
djasper added a reviewer: rsmith.

All use declarations need to be directly placed in the top-level module anyway, 
knowing the submodule doesn't really help. The header that has the offending 
#include can easily be seen in the diagnostics source location.


https://reviews.llvm.org/D43673

Files:
  lib/Lex/ModuleMap.cpp
  test/Modules/Inputs/declare-use/h.h


Index: test/Modules/Inputs/declare-use/h.h
===
--- test/Modules/Inputs/declare-use/h.h
+++ test/Modules/Inputs/declare-use/h.h
@@ -1,7 +1,7 @@
 #ifndef H_H
 #define H_H
 #include "c.h"
-#include "d.h" // expected-error {{does not depend on a module exporting}}
+#include "d.h" // expected-error {{module XH does not depend on a module 
exporting}}
 #include "h1.h"
 const int h1 = aux_h*c*7*d;
 #endif
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -475,7 +475,7 @@
   // We have found a module, but we don't use it.
   if (NotUsed) {
 Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
 return;
   }
 
@@ -486,7 +486,7 @@
 
   if (LangOpts.ModulesStrictDeclUse) {
 Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
   } else if (RequestingModule && RequestingModuleIsModuleInterface &&
  LangOpts.isCompilingModule()) {
 // Do not diagnose when we are not compiling a module. 


Index: test/Modules/Inputs/declare-use/h.h
===
--- test/Modules/Inputs/declare-use/h.h
+++ test/Modules/Inputs/declare-use/h.h
@@ -1,7 +1,7 @@
 #ifndef H_H
 #define H_H
 #include "c.h"
-#include "d.h" // expected-error {{does not depend on a module exporting}}
+#include "d.h" // expected-error {{module XH does not depend on a module exporting}}
 #include "h1.h"
 const int h1 = aux_h*c*7*d;
 #endif
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -475,7 +475,7 @@
   // We have found a module, but we don't use it.
   if (NotUsed) {
 Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
 return;
   }
 
@@ -486,7 +486,7 @@
 
   if (LangOpts.ModulesStrictDeclUse) {
 Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
   } else if (RequestingModule && RequestingModuleIsModuleInterface &&
  LangOpts.isCompilingModule()) {
 // Do not diagnose when we are not compiling a module. 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43598: [clang-format] Tidy up new API guessLanguage()

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Thank you for doing these follow up changes!


Repository:
  rC Clang

https://reviews.llvm.org/D43598



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43522: [clang-format] New API guessLanguage()

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;

benhamilton wrote:
> djasper wrote:
> > In LLVM, we generally don't add braces for single statement ifs.
> Mmmm.. is this a hard requirement? I've personally been bitten so many times 
> by adding statements after missing braces, I'd rather add them unless they're 
> required to not be there by the style guide.
Yes. This is done as consistently as possible throughout LLVM and Clang and I'd 
like to keep clang-format's codebase consistent.

clang-format itself makes it really hard to get bitten by missing braces :).



Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }

benhamilton wrote:
> djasper wrote:
> > Why not just return here?
> I don't like early returns in case an else sneaks in later:
> 
> https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
But I would argue exactly the opposite. At this point, you have pretty uniquely 
determined that this is ObjC (from this originally being viewed as LK_Cpp). Now 
suppose you add additional logic before the return statement in line 2313: That 
would make the state space that this function can have quite complex.

I would even go one step further and completely eliminate the variable 
"result". That would be slightly less efficient, so maybe I'd be ok with:

  const auto GuessFromFilename = getLanguageByFileName(FileName);

And then you can return this at the end or have early exits / other code paths 
if you come up with different languages. Having both, a complex control flow 
and state in local variables seems unnecessarily complex here.



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
 
+struct GuessLanguageTestCase {
+  const char *const FileName;

benhamilton wrote:
> djasper wrote:
> > IMO, this is a bit over-engineered. IIUC, this only calls a single free 
> > standing function with two different parameters and expects a certain 
> > result. You don't need this struct, a test fixture or parameterized tests 
> > for that. Just write:
> > 
> >   TEST(GuessLanguageTest, FileAndCode) {
> > EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
> > EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
> > ...
> >   }
> > 
> > Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I think 
> > that's actually good here. It makes the tests intuitively readable.
> I hear you. I much prefer it when a single test tests a single issue, so 
> failures are isolated to the actual change:
> 
> https://elgaard.blog/2011/02/06/multiple-asserts-in-a-single-unit-test-method/
> 
> If this isn't a hard requirement, I'd like to keep this the way it is.
It certainly makes sense for asserts, as a tests stops upon finding the first 
assert. But these are expectations. Each failing expectation will be reported 
individually, with a direct reference to the line in question and an easily 
understandable error message.

I understand what you are saying but I think my proposal will actually make 
test failures easier to diagnose and understand. Please do change it.


Repository:
  rL LLVM

https://reviews.llvm.org/D43522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43522: [clang-format] New API guessLanguage()

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: cfe/trunk/lib/Format/Format.cpp:2298
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
+  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
+  if (result == FormatStyle::LK_Cpp) {

Here and in several other places: Variables should be named with upper camel 
case 
(https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).



Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;

In LLVM, we generally don't add braces for single statement ifs.



Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }

Why not just return here?



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
 
+struct GuessLanguageTestCase {
+  const char *const FileName;

IMO, this is a bit over-engineered. IIUC, this only calls a single free 
standing function with two different parameters and expects a certain result. 
You don't need this struct, a test fixture or parameterized tests for that. 
Just write:

  TEST(GuessLanguageTest, FileAndCode) {
EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
...
  }

Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I think 
that's actually good here. It makes the tests intuitively readable.


Repository:
  rL LLVM

https://reviews.llvm.org/D43522



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43290: clang-format: tweak formatting of variable initialization blocks

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

In https://reviews.llvm.org/D43290#1008647, @Typz wrote:

> Tweaking the penalty handling would still be needed in any case, since the 
> problem happens also when Cpp11BracedListStyle is true (though the effect is 
> more subtle)


I don't understand this. Which style do you actually care about? With 
Cpp11BracedListStyle=true or =false?

> Generally, I think it is better to just rely on penalties, since it gives a 
> way to compare and weight each solution. Then each style can decide what 
> should break first: e.g. a style may also have a lower 
> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be 
> expected...

And with my reasoning, I think exactly the opposite. Penalties are nice, but if 
the behavior is generally unwanted, than it's very hard to predict in which 
situations it might still occur.


Repository:
  rC Clang

https://reviews.llvm.org/D43290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Please given an explanation of what you are trying to achieve with this change. 
Do you intend to set the penalty high so that clang-format does other things 
first before falling back to wrapping template declarations?

Why treat separate declarations differently wrt. wrapping the template 
declarations? Will this stop at classes vs. functions? I generally have doubts 
that this option carries it's weight. Do you have a style guide that explicitly 
tells you to do this?


Repository:
  rC Clang

https://reviews.llvm.org/D42684



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43440: clang-format: [JS] fix `of` detection.

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D43440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits