[PATCH] D32480: clang-format: Add CompactNamespaces option

djasper added inline comments. Comment at: include/clang/Format/Format.h:358 + /// \endcode + bool BinPackNamespaces; + Typz wrote: > djasper wrote: > > Typz wrote: > > > djasper wrote: > > > > This is not bin packing at all. Maybe CompactNamespaces? Or > > >

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

djasper added a comment. Hm, can't really remember what I meant by "my comment". Probably not important. So, I still see two problems: - I would not count the link you mentioned as a publicly accessible style guide. - I don't think the naming and granularity of this option is right. You

[PATCH] D33394: [clang-format] Keep trailing preprocessor line comments separate from the following section comments

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/UnwrappedLineParser.cpp:106 + isLineComment(*Token) && Token->NewlinesBefore == 1 && + Token->OriginalColumn ==

[PATCH] D32480: clang-format: Add CompactNamespaces option

djasper added a comment. What I mean is that you should remove the CompactNamespace option and instead let this be configured by an additional enum value in NamespaceIndentation. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list

[PATCH] D33394: [clang-format] Keep trailing preprocessor line comments separate from the following section comments

djasper added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:106 + isLineComment(*Token) && Token->NewlinesBefore == 1 && + Token->OriginalColumn == PreviousToken->OriginalColumn); } Any chance of moving this logic to

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

djasper added a comment. In https://reviews.llvm.org/D32478#759347, @Typz wrote: > In https://reviews.llvm.org/D32478#758258, @djasper wrote: > > > When you say "this doesn't happen in tests", do you mean this never happens > > when there are parentheses around the expression? > > > By 'test' I

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

djasper added a comment. Yes, this definitely does not belong in the NamespaceEndCommentsFixer. It has nothing to do with comments. And I am also very skeptical about several things: - Why start here? There are many places where semicolons could be cleaned up. - If we add more of such cleanup

[PATCH] D32480: clang-format: Add CompactNamespaces option

djasper added inline comments. Comment at: include/clang/Format/Format.h:358 + /// \endcode + bool BinPackNamespaces; + Typz wrote: > djasper wrote: > > This is not bin packing at all. Maybe CompactNamespaces? Or > > SingleLineNamespaces? > > > > Also, could

[PATCH] D33640: clang-format: [JS] fix indenting bound functions.

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/ContinuationIndenter.cpp:210 // FIXME: We should find a more generic solution to this problem. - !(State.Column <= NewLineColumn &&

[PATCH] D34238: clang-format: Do not binpack initialization lists

djasper added a comment. I am fine not bin-packing when the last element has a trailing comma. But lets not special case assignments. https://reviews.llvm.org/D34238 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34330: [clang-format] handle `if constexpr`

djasper closed this revision. djasper added a comment. Yes, I saw. As this version seems to handle the one-line case correctly, I submitted this one as r305666. https://reviews.llvm.org/D34330 ___ cfe-commits mailing list

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

djasper closed this revision. djasper added a comment. Renamed Tok to RecordTok to avoid the nested scope and submitted as r305667. https://reviews.llvm.org/D32825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33491: clang-format: Fix C99 designated initializers corner cases

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:339 Left->Type = TT_JsComputedPropertyName; + } else if ((Style.Language == FormatStyle::LK_Cpp || + Style.Language == FormatStyle::LK_ObjC) && Use

[PATCH] D34330: [clang-format] handle `if constexpr`

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

[PATCH] D26953: clang-format: handle formatting on constexpr if

djasper closed this revision. djasper added a comment. Submitted the other implementation of this as r305666. https://reviews.llvm.org/D26953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32480: clang-format: Add CompactNamespaces option

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Yeah, looks good. Krasimir, any further concerns? https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32480: clang-format: Add CompactNamespaces option

djasper added a comment. Ok. Works for me. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:523 + if (C.NewlinesBefore > 0 && C.ContinuesPPDirective) +C.EscapedNewlineColumn = C.PreviousEndOfTokenColumn + 2; +return; I think we should not duplicate this loop.

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. This is an edge case in actual C++. An escaped newline literally gets expanded to nothing. So what this reads is #define Onetwo \ ... https://reviews.llvm.org/D32733

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

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

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

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

[PATCH] D32864: clang-format: [JS] exponentiation operator

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: unittests/Format/FormatTestJS.cpp:1791 +TEST_F(FormatTestJS, Exponentiation) { + verifyFormat("squared = x ** 2;"); +} Also make this

[PATCH] D32864: clang-format: [JS] exponentiation operator

djasper accepted this revision. djasper added a comment. Thanks https://reviews.llvm.org/D32864 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace

djasper added a comment. I think we should just not do this for now. This addresses a very infrequent case that's easy enough to fix manually. As such, it's not worth the added complexity of clang-format and potential failures it might generate. Changing non-whitespace/non-comment code is

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

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

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

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

[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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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. 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] 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] 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 != ) +--MatchingStackIndex; I think this needs a long

[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

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

[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

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

[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

[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

[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

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

<    1   2   3   4   5   >