[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 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 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] 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] 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] 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] 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] 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. 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] 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] 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 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. 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] 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] 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] 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] D37192: [clang-format] Add support for generic Obj-C categories

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Comment at: lib/Format/UnwrappedLineParser.cpp:2099 + // After a protocol list, we can have a category (Obj-C generic + // category). nit:

[PATCH] D37142: clang-format: [JS] simplify template string wrapping.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Yay for *removing* complexity for a change :). Let me know how it goes in practice. https://reviews.llvm.org/D37142 ___ cfe-commits mailing lis

[PATCH] D37142: clang-format: [JS] simplify template string wrapping.

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1139 + + // On lines containing template strings, propagate NoLineBreak even for dict + // and array literals. This is to force wrapping an initial function call if This is not the r

[PATCH] D37142: clang-format: [JS] simplify template string wrapping.

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1139 + + // On lines containing template strings, propagate NoLineBreak even for dict + // and array literals. This is to force wrapping an initial function call if mprobst wrote: >

[PATCH] D37143: [clang-format] Fixed typedef enum brace wrapping

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Thank you. https://reviews.llvm.org/D37143 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D37132: [clang-format] Add support for C++17 structured bindings.

djasper added a comment. Are you changing the line endings here? Phabricator tells me that basically all the lines change. If so, please don't ;). Comment at: lib/Format/TokenAnnotator.cpp:345 + +FormatToken *PreviousNoneOfConstVolatileReference = Parent; +while (Previ

[PATCH] D37136: [clang-format] Do not format likely xml

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Just a few minor comments, otherwise looks good. Comment at: lib/Format/Format.cpp:1542 +bool likelyXml(StringRef Code) { + return Code.ltrim().startswith("<");

[PATCH] D37513: [clang-format] Fix documentation for AllowAllParametersOfDeclarationOnNextLine

djasper added a comment. Note that these changes need to be made to the corresponding comments in include/clang/Format/Format.h and then this file is auto-generated with docs/tools/dump_format_style.py. Comment at: docs/ClangFormatStyleOptions.rst:274 **AllowAllParametersOfD

[PATCH] D37531: Add an usage example of BreakBeforeBraces

djasper added a comment. This change needs to be made to include/clang/Format/Format.h and then the rst file needs to be regenerated with docs/tools/dump_format_style.py. https://reviews.llvm.org/D37531 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D37531: Add an usage example of BreakBeforeBraces

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D37531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D37558: Refresh the clang format options doc with the recent changes

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Thank you. https://reviews.llvm.org/D37558 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D37513: [clang-format] Fix documentation for AllowAllParametersOfDeclarationOnNextLine

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Also run dump_format_style.py and keep the changed .rst file in this change. Otherwise looks good. https://reviews.llvm.org/D37513 ___ cfe-com

[PATCH] D37132: [clang-format] Add support for C++17 structured bindings.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Sorry for the delay. https://reviews.llvm.org/D37132 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

[PATCH] D37513: [clang-format] Fix documentation for AllowAllParametersOfDeclarationOnNextLine

djasper accepted this revision. djasper added a comment. Submitted as r312721. Thank you. https://reviews.llvm.org/D37513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37260: [clang-format] Fixed extern C brace wrapping

djasper added a comment. I am very strongly against a flag that just leaves the line break as is. What's the motivation? https://reviews.llvm.org/D37260 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

djasper added a comment. I have a slightly hard time grasping what this patch now actually does? Doesn't it simply try to decide whether or not to make a split locally be comparing the PenaltyBreakComment against the penalty for the access characters? If so, couldn't we simply do that as an imp

[PATCH] D37260: [clang-format] Fixed extern C brace wrapping

djasper added a comment. BraceWrapping.AfterExternC makes sense to me. https://reviews.llvm.org/D37260 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33440: clang-format: better handle statement and namespace macros

djasper added a comment. I'd still prefer individual patches for each of these changes. If the code review system or VCS make it hard for you to deal with two adjacent changes this way, do them in sequence. Adding Manuel as a reviewer who has a longer term idea on how to handle macros. https:

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

djasper added a comment. I still don't understand yet. breakProtrudingToken has basically two options: 1. Don't wrap/reflow: In this case the penalty is determined by the number of excess characters. 2. Wrap/reflow: I this case the penalty is determined by PenaltySplitComments plus the remainin

[PATCH] D33440: clang-format: better handle statement macros

djasper added inline comments. Comment at: lib/Format/FormatTokenLexer.cpp:642 tok::pp_define) && -std::find(ForEachMacros.begin(), ForEachMacros.end(), - FormatTok->Tok.getIdentifierInfo()) != ForEachMacros.end()) { - FormatTok->Type

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

djasper added a comment. I think doing the computation twice is fine. Or at least, I'd need a test case where it actually shows substantial overhead before doing what you are doing here. Understand that creating more States and making the State object itself larger also has cost and that cost o

[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] 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] 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. 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] D40424: clang-format: [JS] handle semis in generic types.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D40424 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D40642: clang-format: [JS] do not wrap after async/await.

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/D40642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.o

[PATCH] D40909: [clang-format] Reorganize raw string delimiters

djasper added inline comments. Comment at: include/clang/Format/Format.h:1216 +LK_TextProto, +/// Do not use. Keep at last position. +LK_End, Lets find a way to implement without this in the public header file. Comment at: include/c

[PATCH] D40909: [clang-format] Reorganize raw string delimiters

djasper added inline comments. Comment at: include/clang/Format/Format.h:1375 +std::vector EnclosingFunctionNames; +/// \brief The canonical delimiter for this language. +std::string CanonicalDelimiter; krasimir wrote: > djasper wrote: > > Can you pul

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

djasper added a comment. Could you explain this in more detail? Which subtraction is underflowing? Why can't we just add a ternary expression there to handle the case? https://reviews.llvm.org/D36019 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D35743: [clang-format] Handle Structured binding declaration in C++17

djasper added a comment. Please add all the tests into unittests/Format/FormatTest.cpp instead. We use FileCheck-based tests only to verify the behavior of the clang-format binary itself. https://reviews.llvm.org/D35743 ___ cfe-commits mailing lis

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: docs/ClangFormatStyleOptions.rst:1182 +**IndentPPDirectives** (``bool``) + Indent preprocessor directives on conditionals. I think we can foresee that a bool is not going to be enough here. Make this an enum, which f

[PATCH] D36131: clang-format: [JS] handle object types in extends positions.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D36131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D36132: clang-format: [JS] support default imports.

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/SortJavaScriptImports.cpp:416 break; - if (Current->isNot(tok::identifier)) + if (Current->isNot(tok::identifier) && Current->

[PATCH] D36144: clang-format: [JS] consistenly format enums.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D36144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D36146: clang-format: [JS] whitespace between keywords and parenthesized expressions.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D36146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D36147: clang-format: [JS] handle union types in arrow functions.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D36147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2355 +(Left.Tok.getIdentifierInfo() || + Left.isOneOf(tok::kw_switch, tok::kw_case, tok::kw_delete))) + return false; Why is instanceof not required in this list?

[PATCH] D36139: clang-format: [JS] prefer wrapping chains over empty literals.

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2009 +// Prefer breaking call chains (".foo") over empty "{}", "[]" or "()". +if ((Left.is(tok::l_brace) && Right.is(tok::r_brace)) || +(Left.is(tok::l_square) && Right.is(tok::r_square)) ||

[PATCH] D35743: [clang-format] Handle Structured binding declaration in C++17

djasper added a comment. Generally, please upload patches with full context to phabricator. (or use arc) I think this approach is a bit inside out. We are in a codepath where we try to find a lambda introducer and we the look one or two tokens back to see that we aren't as we have seen "auto".

[PATCH] D36148: clang-format: [JS] support fields with case/switch/default labels.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D36148 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:383 + Current.Previous->is(tok::hash) && State.FirstIndent > 0) { +// subtract 1 so indent lines up with non-preprocessor code +Spaces += State.FirstIndent; euhlmann wrote

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:650 +for (unsigned i = 0; i < Newlines; ++i) + Text.append(UseCRLF ? " \\\r\n" : " \\\n"); +return; Note that when you have an empty line, this would turn into: #define A

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Thanks you. Comment at: lib/Format/WhitespaceManager.cpp:650 +for (unsigned i = 0; i < Newlines; ++i) + Text.append(UseCRLF ? " \\\r\n" : " \\\n"); +return; --

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added a comment. Manuel: Can you take a look at the last comment here? Why does PPBranchLevel start at -1? https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D36491: clang-format: [JS] detect ASI after closing parens.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D36491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D34324: [clang-format] let PointerAlignment dictate spacing of function ref qualifiers

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Thank you! https://reviews.llvm.org/D34324 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D36684: clang-format: [JS] wrap optional properties in type aliases.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D36684 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D36142: clang-format: [JS] do not insert whitespace in call positions.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D36142 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: unittests/Format/FormatTest.cpp:2281 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) { verifyFormat("#define A \\\n" mzeren-vmw wrote: > mzeren-vmw wrote: > > Experimenting with the patch locally

[PATCH] D36956: [clang-format] Emit absolute splits before lines for comments

djasper added inline comments. Comment at: lib/Format/BreakableToken.cpp:553 StringRef TrimmedContent = Content[LineIndex].ltrim(Blanks); - return getReflowSplit(TrimmedContent, ReflowPrefix, PreviousEndColumn, -ColumnLimit); + Split TrimmedSplit = ge

[PATCH] D36614: [clang-format] Refine trailing comment detection

djasper added inline comments. Comment at: lib/Format/FormatToken.h:397 + (!Previous || + Previous->isOneOf(tok::comma, tok::equal, tok::l_brace) || + Next->is(tok::r_brace; Is this list coming from existing tests?

[PATCH] D37011: [clang-format] Fix lines regression in clang-format.py

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Thank you https://reviews.llvm.org/D37011 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D37007: [clang-format] Break non-trailing block comments

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. If no tests break with this, lets just go for it. We can follow up and fix individual cases if we find undesired behavior. https://reviews.llvm.org/D37007 ___

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added a comment. Krasimir: Can you actually give this a round of review? I will also try to do so tomorrow. https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D36956: [clang-format] Emit absolute splits before lines for comments

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D36956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: unittests/Format/FormatTest.cpp:2297 + "#include \n" + "#define MACRO " + "\\\n" Please just set Style.ColumnLim

[PATCH] D37109: [clang-format] Emit absolute splits before lines for comments, try 2

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Does the test still test the same thing if you set the column limit to 60 and remove 20 spaces? If not, this is fine. https://reviews.llvm.org/D37109

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. From my side this looks good for now (we can always improve more later). Krasimir, what do you think? https://reviews.llvm.org/D35955 ___ cfe-

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

<    1   2   3   4   5   >