[PATCH] D88239: [clang-format] Fix spaces around */& in multi-variable declarations
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.
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
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
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.
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 + + verif
[PATCH] D48363: [clang-format] Enable text proto formatting in common functions
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
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
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
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
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
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
djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:44 + int MatchingStackIndex = Stack.size() - 1; + while (MatchingStackIndex >= 0 && Stack[MatchingStackIndex].Tok != &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 != &Tok) --MatchingStackIndex; return MatchingStackIndex >= 0 ? &Stack[MatchingStackIndex] : nullptr; }; Then you could write the rest as: ... FindParenState(&Tok); 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
djasper added inline comments. Comment at: lib/Format/Format.cpp:1449 const AdditionalKeywords &Keywords) { +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 &Line, + const AdditionalKeywords &Keywords) { 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
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
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
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 &Line : AnnotatedLines) { - for (FormatToken *FormatTok = Line->First; FormatTok; +auto CheckLineTokens = [&Keywords](const AnnotatedLine &Line) { + 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.
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
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
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
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
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
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
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
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
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
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
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 &Tok, + 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
djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:152 + const FormatToken *Next = CurrentToken->getNextNonComment(); + int ParenDepth = 1; + // Handle nested parens in case we have an array of blocks with No. Don't implement yet another parenthesis counting. This function already iterates over all tokens until the closing paren. Just store a pointer to the caret here and mark it (or unmark it) when encountering the closing brace (line 271). There is already very similar logic there to set TT_FunctionTypeLParen (which is actually doing the exact same parsing now that I think of 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] D43906: [clang-format] Improve detection of Objective-C block types
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
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
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
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
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
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
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
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
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
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.
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
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
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
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.
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
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
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
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.
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
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
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
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
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
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()
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()
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()
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
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.
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