[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.

[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 SINC

[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 __

[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.:

[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

[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:

[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. Howev

[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://

[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

[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

[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 sus

[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 do

[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 combi

[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.o

[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 BreakBefore

[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

[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

[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. Ho

[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: > djaspe

[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.o

[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 ac

[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.or

[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-c

[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 suspiciou

[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 = FindParenSta

[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 explan

[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://l

[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 n

[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.

[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 t

[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

[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-co

[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.In

[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.o

[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

[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 _

[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 ca

[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 yo

[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://l

[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.Previo

[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 lan

[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

[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.o

[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

[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 && ---

[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

[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.

[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:

[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 tha

[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

[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.o

[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.Matching

[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 her

[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

[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 s

[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; +a

[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 temp

[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.Cpp11Braced

[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://re

[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 wr

[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 growi

[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 ___

[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 = !Co

[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

[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

[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@lis

[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))) { --

[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))) { --

[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_squar

[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 rememb

[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/T

[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 'def

[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 co

[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

[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

[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 circ

[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(

[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

[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 aaa

[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 BreakI

[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,

[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

[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.or

[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

[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/

[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.

[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://

[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@l

[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 sin

[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) { --

[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

[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. wra

  1   2   3   4   5   >