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

djasper added a comment. In https://reviews.llvm.org/D43183#1008784, @Typz wrote: > A user can create an error by reasoning like this, after the code has been > formatted this way (a long time ago) : "oh, I need to make an extra function > call, but in all cases ah, here is the end of the

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

djasper added a comment. That doesn't explain to me how this is error prone. I can't think how you'd create an error by this that would not be caught by the compiler. Much less if you consistently use clang-format. It's fundamentally what you get for IndentCaseLabels: false. Even without

[PATCH] D43298: [clang-format] Support repeated field lists in protos

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:2355 + ((Right.MatchingParen->is(TT_ArrayInitializerLSquare) && +

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

djasper added a comment. Yes, that's what I mean. What do you mean, the style is too error prone? Repository: rC Clang https://reviews.llvm.org/D43183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43303: [Format] Fix for bug 35641

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Thanks for the fix. Comment at: unittests/Format/FormatTest.cpp:9453 + + // Bug 35641 + Alignment.AlignConsecutiveDeclarations = true; Make this "See

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

djasper added a comment. What you are doing makes sense to me. My only hesitation is whether we should maybe completely disallow breaking between = and { when Cpp11BracedListStyle is false instead of solving this via penalties. Specifically, | 80

[PATCH] D43294: [clang-format] Recognize percents as format specifiers in protos

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Ok.. I guess ;) Repository: rC Clang https://reviews.llvm.org/D43294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

djasper added a comment. In https://reviews.llvm.org/D43183#1006224, @Typz wrote: > It is explicitly documented in google style guide: > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements > : > > > case blocks in switch statements can have curly braces or not,

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

djasper added a comment. In https://reviews.llvm.org/D43183#1006170, @Typz wrote: > > We'll just format those cases in a somewhat weird way and users can either > > accept that or change their code to not need it. > > I think we have a really diverging opinion on this. From my experience, >

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

djasper added a comment. To me none of these options make sense. For the case you are describing, I agree that the current behavior is not ideal, but neither are any of the alternatives. However, I think that's fine. We'll just format those cases in a somewhat weird way and users can either

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

djasper added a comment. Do you have a reference to style guides recommending any of this? Repository: rC Clang https://reviews.llvm.org/D43183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43180: [clang-format] Support text proto extensions

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Cool, thanks. Repository: rC Clang https://reviews.llvm.org/D43180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43180: [clang-format] Support text proto extensions

djasper added inline comments. Comment at: unittests/Format/FormatTestTextProto.cpp:317 + +TEST_F(FormatTestTextProto, FormatsExtensions) { + verifyFormat("[type] { key: value }"); It might be useful to attach a test case for what happens if the "[...]" does

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

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/D43121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43121: clang-format: keep ObjC colon alignment with short object name

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:900 + std::max(NextNonComment->LongestObjCSelectorName, + unsigned(NextNonComment->TokenText.size())) - NextNonComment->ColumnWidth; I'd

[PATCH] D42957: [clang-format] Do not break before long string literals in protos

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/D42957 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

djasper added a comment. No. The reason for us generally asking for a style guide is because it unambiguously clarifies the exact style that is to be preferred. Projects that don't have a style guide written down also often do not agree on what the style should be. That said, I think the

[PATCH] D42727: [clang-format] Adds space around angle brackets in text protos

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/D42727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

djasper added a comment. You still haven't addressed my comment about there not being a publicly accessible style guide recommending these. https://reviews.llvm.org/D32525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

djasper added a comment. Generally, upload patches with the full file as diff context. Phabricator hides that by default, but enables me to expand beyond the patch regions (where it currently says "Context not available"). Comment at: lib/Format/ContinuationIndenter.cpp:756

[PATCH] D42727: [clang-format] Adds space around angle brackets in text protos

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:82 CurrentToken->MatchingParen = Left; -CurrentToken->Type = TT_TemplateCloser; +if (Style.Language == FormatStyle::LK_TextProto) + CurrentToken->Type = TT_DictLiteral;

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

djasper added a comment. - 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

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

djasper added a comment. Ah, Manuel and Krasimir are already on this thread, maybe they can comment? I also added Chandler and Sam who I know care about formatting somewhat. Repository: rC Clang https://reviews.llvm.org/D42787 ___ cfe-commits

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

djasper added a comment. I don't mean trivial with a diff. What I mean is, users will find it surprising if whether or not a parameter gets wrapped leads to a different indentation internal to that parameter. I have not heard of a single user that would be surprised by this extra indentation.

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

djasper added a comment. You might doubt it, but having written the code I can tell you that it's the case. Shame on me for not writing a test, though. I see the argument why this indentation is not necessary in exactly the case where the last parameter is multi-line and not wrapped to a new

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

djasper added a comment. I am against this change. The current behavior here is intentional and IMO more consistent. If there is more than one precedence level in a set of parentheses, we add the additional indentation. If you don't like it, surround it with extra parentheses. Generally, it'd

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

djasper added a comment. I don't think cases where there is only a semicolon-less macro are particularly frequent/important either. You probably could even add a semicolon in those cases, right? How frequent are such cases in your codebase? Either way, they aren't fixed by this patch, so they

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

djasper added a comment. I think this case is not important enough to fix. Please tell users to just delete the useless semicolon. In particular, my concern is that this makes the data-flow significantly more complicated. Early on in the development, we had separate token classes for the

[PATCH] D42685: [clang-format] Adds space around braces in text protos

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/D42685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42570: clang-format: [JS] Prevent ASI before [ and (.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Makes sense. Repository: rC Clang https://reviews.llvm.org/D42570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42500: [clang-format] Fixes indentation of inner text proto messages

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/D42500 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42373: [clang-format] Disable string literal breaking for text protos

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Change the comment and possibly also the patch description along the lines of what I have suggested. Other than that, looks good to me. Comment at:

[PATCH] D42376: [clang-format] Ignore UnbreakableTailLength sometimes during breaking

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Happy to go forward with this. I think we might also wanna investigate whether entirely removing UnbreakableTailLength would be beneficial. I think we implemented it as an optimization, but

[PATCH] D42376: [clang-format] Ignore UnbreakableTailLength sometimes during breaking

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1579 (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))) { + unsigned UnbreakableTailLength = (State.NextToken && canBreak(State)) +

[PATCH] D42373: [clang-format] Disable string literal breaking for text protos

djasper added a comment. I am not sure we should actually do this. I agree that badly reflowing multiline string literals is not ideal, but neither is violating the column limit. In any case, proper reflowing would probably the best solution. How hard would it be to implement that (for proto

[PATCH] D42376: [clang-format] Ignore UnbreakableTailLength sometimes during breaking

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1579 (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))) { + unsigned UnbreakableTailLength = (State.NextToken && canBreak(State)) +

[PATCH] D42036: [clang-format] Keep comments aligned to macros

djasper added a comment. While I agree that there is probably a bug to fix, I don't (yet) agree with what is proposed in this patch. I think a comment in between preprocessor directives should always either: - Be considered part of the code in between the #-lines - Be considered to be

[PATCH] D42372: [clang-format] Ignore UnbreakableTailLength sometimes during breaking

djasper added a comment. I think I understand now. I think I'd prefer pulling all of the "+ UnbreakableTailLength" calculations out of getRemainingLength so that you don't have to pass around the new parameter CanBreakAfter. Instead, only add it if necessary outside of the function.

[PATCH] D42372: [clang-format] Ignore UnbreakableTailLength sometimes during breaking

djasper added a comment. I don't understand why the closing braces would count towards the string literals UnbreakableTailLength. Isn't that a bug? Repository: rC Clang https://reviews.llvm.org/D42372 ___ cfe-commits mailing list

[PATCH] D41195: [ClangFormat] IndentWrappedFunctionNames should be true in the google ObjC style

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: unittests/Format/FormatTestObjC.cpp:388 + // Wrapped method parameters should be indented. + verifyFormat("- (VeryLongReturnTypeName)\n" +

[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

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

[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

[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

[PATCH] D40178: clang-format: [JS] remove trailing lines in arrow functions.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. There is a chance that some people do not want this in their coding style. But if so, we can add an option later. https://reviews.llvm.org/D40178

[PATCH] D40178: clang-format: [JS] remove trailing lines in arrow functions.

djasper added a comment. Is this different for C++ lambdas? I would think that we never should add an empty line before the "}" of a child block. https://reviews.llvm.org/D40178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39478: [clang-format] Handle leading comments in using declaration

djasper closed this revision. djasper added a comment. Submitted as r317901. https://reviews.llvm.org/D39478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39806: [clang-format] Support python-style comments in text protos

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/FormatTokenLexer.cpp:344 + size_t To = Lex->getBuffer().find_first_of('\n', From); + if (To == StringRef::npos) To = Lex->getBuffer().size();

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

djasper added a comment. Out of curiosity, will this be able to fix the two situations that you get for python extension? There, you usually have a PyObject_HEAD with out semicolon in a struct and than a PyObject_HEAD_INIT(..) in a braced init list. More info:

[PATCH] D39478: [clang-format] Handle leading comments in using declaration

djasper accepted this revision. djasper added a comment. Looks good. Do you have submit access? https://reviews.llvm.org/D39478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39587: [clang-format] Handle unary operator overload with arguments and specifiers

djasper added a comment. Submitted as r317473. Thank you! https://reviews.llvm.org/D39587 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39587: [clang-format] Handle unary operator overload with arguments and specifiers

djasper added a comment. Looks good, thank you. https://reviews.llvm.org/D39587 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39478: [clang-format] Handle leading comments in using declaration

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Some minor remarks, but generally looks good. Thanks for fixing this! Comment at: lib/Format/UsingDeclarationsSorter.cpp:136 for (size_t I = 0, E =

[PATCH] D39024: [clang-format] Sort whole block of using declarations while partially formatting

djasper added inline comments. Comment at: lib/Format/UsingDeclarationsSorter.cpp:79 const SourceManager , tooling::Replacements *Fixes) { - SmallVector SortedUsingDeclarations( - UsingDeclarations->begin(), UsingDeclarations->end()); -

[PATCH] D37695: [clang-format] Break non-trailing comments, try 2

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. looks good. https://reviews.llvm.org/D37695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37695: [clang-format] Break non-trailing comments, try 2

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.h:270 + // \c breakProtrudingToken. + bool LastBlockCommentWasBroken : 1; + We should be *very* careful when adding stuff to ParenState as every extra bit of data and every extra

[PATCH] D38243: [clang-format] Add ext/ to google include categories

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

[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

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

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

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

[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

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

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

[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

[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

[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

[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

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

[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

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

[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

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

[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 added inline comments. Comment at: unittests/Format/FormatTest.cpp:2297 + "#include \n" + "#define MACRO " + "\\\n" Please just set

[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

[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

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

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

[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

[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

[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

[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

[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

[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

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

[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

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

<    1   2   3   4   5   >