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

2018-01-23 Thread Daniel Jasper via Phabricator via cfe-commits
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 as

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

2018-01-23 Thread Daniel Jasper via Phabricator via cfe-commits
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] D42376: [clang-format] Ignore UnbreakableTailLength sometimes during breaking

2018-01-23 Thread Daniel Jasper via Phabricator via cfe-commits
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] D42373: [clang-format] Disable string literal breaking for text protos

2018-01-24 Thread Daniel Jasper via Phabricator via cfe-commits
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: lib/Format/Format.cpp:6

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

2018-01-24 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.llvm.or

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

2018-01-26 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.llvm.

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

2018-01-30 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.llvm.o

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

2018-01-31 Thread Daniel Jasper via Phabricator via cfe-commits
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 diffe

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

2018-01-31 Thread Daniel Jasper via Phabricator via cfe-commits
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 a

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

2018-02-01 Thread Daniel Jasper via Phabricator via cfe-commits
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] D42787: clang-format: do not add extra indent when wrapping last parameter

2018-02-01 Thread Daniel Jasper via Phabricator via cfe-commits
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 li

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

2018-02-01 Thread Daniel Jasper via Phabricator via cfe-commits
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

2018-02-01 Thread Daniel Jasper via Phabricator via cfe-commits
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 ma

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

2018-02-01 Thread Daniel Jasper via Phabricator via cfe-commits
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 if

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

2018-02-02 Thread Daniel Jasper via Phabricator via cfe-commits
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] D40988: Clang-format: add finer-grained options for putting all arguments on one line

2018-02-05 Thread Daniel Jasper via Phabricator via cfe-commits
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] D32525: [clang-format] Add SpaceBeforeColon option

2018-02-06 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.llvm.or

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

2018-02-06 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.llvm.o

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

2018-02-06 Thread Daniel Jasper via Phabricator via cfe-commits
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 style

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

2017-05-08 Thread Daniel Jasper via Phabricator via cfe-commits
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

2017-05-08 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.llvm.org/cgi-bin/mai

[PATCH] D32997: clang-format: [JS] keep triple slash directives intact.

2017-05-09 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D32997 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D33029: [clang-format] add option for dangling parenthesis

2017-05-09 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Please read and address: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D33006: clang-format: refine calculating brace types.

2017-05-10 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D33006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

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

2017-05-10 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. One nit, otherwise looks good. Comment at: lib/Format/UnwrappedLineFormatter.cpp:368 // We don't merge short records. - if (Line.First->isOneOf(tok::kw_class, t

[PATCH] D33029: [clang-format] add option for dangling parenthesis

2017-05-12 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Probably all of the examples from the original patch description and later comments should be turned into unit tests. Comment at: docs/ClangFormatStyleOptions.rst:953 +**DanglingParenthesis** (``bool``) + If there is a break after the opening parent

[PATCH] D33023: clang-format: [JS] wrap params with trailing commas.

2017-05-15 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1043 +bool EndsInComma = +Current.MatchingParen && Please make this specific to JavaScript for now. C++ doesn't allow trailing commas here and a trailing comma is more l

[PATCH] D33193: clang-format: [JS] for async loops.

2017-05-15 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/TokenAnnotator.cpp:583 Contexts.back().ColonIsForRangeExpr = true; + // for async ( ... + if (CurrentToken->is(Keywords.kw_async

[PATCH] D33238: [clang-format] Make NoLineBreakFormatter respect MustBreakBefore

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Are there cases where this makes a difference? If so, add a test. If not, add something to the patch description to clarify. Comment at: lib/Format/TokenAnnotator.cpp:2473

[PATCH] D32477: [clang-format] Allow customizing the penalty for breaking assignment

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Basically looks good, but please add a test case where the penalty is high show that it changes behavior. https://reviews.llvm.org/D32477 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

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

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. The current behavior here is actually intended. If you'd like the other format, we probably need to add a style option. What style guide are you basing this on? Comment at: unittests/Format/FormatTest.cpp:2476 "bool value = a

[PATCH] D32479: [clang-format] Add BreakConstructorInitializersBeforeColon option

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Yes, turning that option into an enum seems like the better choice here. https://reviews.llvm.org/D32479 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Format/Format.h:153 + /// \endcode + bool AllowSemicolonAfterNamespace; + While I am not entirely opposed to this feature, I think it should be a separate patch. Comment at: include/cl

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

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I have looked through the PDF you linked to, but I couldn't really find much about formatting. I have found one instance of a constructor initializer list, but there is no accompanying text saying that this is even intentional. Haven't found a range-based for loop. As s

[PATCH] D33029: [clang-format] add option for dangling parenthesis

2017-05-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Format/Format.h:793 + /// \endcode + bool DanglingParenthesis; + stringham wrote: > djasper wrote: > > I don't think this is a name that anyone will intuitively understand. I > > understand that the nami

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

2017-05-18 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. When you say "this doesn't happen in tests", do you mean this never happens when there are parentheses around the expression? Comment at: unittests/Format/FormatTest.cpp:2476 "bool value = a\n" -

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

2017-05-18 Thread Daniel Jasper via Phabricator via cfe-commits
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 f

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

2017-05-18 Thread Daniel Jasper via Phabricator via cfe-commits
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] D33314: clang-format: Add option to remove semicolon at end of namespace

2017-05-19 Thread Daniel Jasper via Phabricator via cfe-commits
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 alway

[PATCH] D33351: [clang-format] Handle trailing comment sections in import statement lines

2017-05-19 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. nice Comment at: lib/Format/ContinuationIndenter.cpp:590 1u, std::min(Current.NewlinesBefore, Style.MaxEmptyLinesToKeep + 1)); +bool InPPDirective = +S

[PATCH] D32479: clang-format: Introduce BreakConstructorInitializers option

2017-05-22 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Format/Format.h:699 + /// This option is **deprecated* and is retained for backwards compatibility. bool BreakConstructorInitializersBeforeComma; I don't think you need to keep this around. The YAML p

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

2017-05-22 Thread Daniel Jasper via Phabricator via cfe-commits
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] D33394: [clang-format] Keep trailing preprocessor line comments separate from the following section comments

2017-05-22 Thread Daniel Jasper via Phabricator via cfe-commits
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 d

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

2017-05-22 Thread Daniel Jasper via Phabricator via cfe-commits
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 specifi

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

2017-05-22 Thread Daniel Jasper via Phabricator via cfe-commits
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 cfe-comm

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

2017-05-22 Thread Daniel Jasper via Phabricator via cfe-commits
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 == PreviousT

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

2017-05-23 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. That's what I meant by "The name NamespaceIndentation might then be a bit confusing, but not sure whether it's worth changing it.". I am honestly not sure. Let's get a third opinion. If we add this additional option, I think we need to fix the behavior and make what pe

[PATCH] D32479: clang-format: Introduce BreakConstructorInitializers option

2017-05-23 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:196 + FormatStyle::BCIS_AfterColonAndComma) && + (State.Column + State.Line->Last->TotalLength - Previous.TotalLength > + getColumnLimit(State) || Typz wrote:

[PATCH] D32479: clang-format: Introduce BreakConstructorInitializers option

2017-05-23 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:196 + FormatStyle::BCIS_AfterColonAndComma) && + (State.Column + State.Line->Last->TotalLength - Previous.TotalLength > + getColumnLimit(State) || Typz wrote:

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

2017-05-23 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Don't C99 designated literals use "=" instead of ":"? https://reviews.llvm.org/D32525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Does anything speak against making this behavior happen with AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra style option? Comment at: unittests/Format/FormatTest.cpp:6067 +

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. As it currently stands, I am really not happy with the configuration space that this opens up. If we can't make the configuration of existing flags, what's the coding style encourages this behavior? https://reviews.llvm.org/D33447 ___

[PATCH] D32479: clang-format: Introduce BreakConstructorInitializers option

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
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/ContinuationIndenter.cpp:196 + FormatStyle::BCIS_AfterColonAndComma) && + (State.Column + State.Line->Last->Tot

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. But that style specifically says that it is only done if the initializer list is wrapped: https://github.com/facebook/hhvm/blob/master/hphp/doc/coding-conventions.md#constructor-initializer-lists I.e. we would do the right thing for that style if we would set BraceWrapp

[PATCH] D33440: clang-format: properly handle Q_UNUSED and QT_REQUIRE_VERSION

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. clang-format already has logic to detect semicolon-less macro invocations an in fact this already does behave as I would expect. What are you fixing? https://reviews.llvm.org/D33440 ___ cfe-commits mailing list cfe-commits@

[PATCH] D33440: clang-format: properly handle Q_UNUSED and QT_REQUIRE_VERSION

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I don't. Only if they start out to be on the same line. As long as I start with: class C { void foo(int a, int b) { Q_UNUSED(a) Q_UNUSED(a) return b; } }; clang-format leaves this alone. That's good enough I think and we don't want to add m

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. No, I don't think it should be done this way and neither Facebook nor Mozilla coding styles say you should. Mozilla style has an explicit example: int TinyFunction() { return mVar; } Facebook style has an explicit example: MyClass::MyClass(uint64_t idx) : m_idx(id

[PATCH] D33440: clang-format: properly handle Q_UNUSED and QT_REQUIRE_VERSION

2017-05-24 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I generally would not be opposed to such a patch. However, note that this might be hard to get right. We had significant performance problems in the past with ForEachMacros as we used to match every single identifier against the regex stored in there. For for loops you

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

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
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] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. In all honesty, I think this style isn't thought out well enough. It really is a special case for only "=" and "return" and even there, it has many cases where it simply doesn't make sense. And then you have cases like this: bool = aa // == // &&

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

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. In https://reviews.llvm.org/D32478#765548, @Typz wrote: > In https://reviews.llvm.org/D32478#765537, @djasper wrote: > > > In all honesty, I think this style isn't thought out well enough. It really > > is a special case for only "=" and "return" and even there, it has m

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I don't understand. WebKit style would not set AllowShortFunctionsOnASingleLine and so the behavior there wouldn't change, I presume? https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

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

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. In https://reviews.llvm.org/D32478#765583, @Typz wrote: > I actually don't know how, but it still manages somehow : I rebuilt this > exact patch to ensure I gave you the correct output. > And the same behavior can be seen in the test cases, where the operator with > hi

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong: MyOtherClass::MyOtherClass() : MySuperClass() {} So I still think we can make this work with the existing options without regressing a behavior that anyone is relyi

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

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. In https://reviews.llvm.org/D32478#765642, @Typz wrote: > Nop, it's formatted like this: > > bool a = aa // > == // > && c; > > bool a = aa // > == // >+ c; > > > The current way to format

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Lets try this the other way around. I am not ok with introducing an additional top-level option for this. It simply isn't important enough. So find a way for the existing style flags to support what you need and not regress existing users. If that can't be done, I am al

[PATCH] D33447: clang-format: add option to merge empty function body

2017-05-26 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Format/Format.h:644 +/// This option is used only if the opening brace of the function has +/// already +/// been wrapped, i.e. the `AfterFunction` brace wrapping mode is set, and Reflow the com

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

2017-05-29 Thread Daniel Jasper via Phabricator via cfe-commits
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 && P

[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.

2016-11-30 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:41 + /// \brief Creates an edit list for a key position. + EditList(const SourceManager &SM, SourceLocation KeyPosition); + I wonder whether we should always use a SourceLoc

[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.

2016-11-30 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:41 + /// \brief Creates an edit list for a key position. + EditList(const SourceManager &SM, SourceLocation KeyPosition); + ioeric wrote: > ioeric wrote: > > klimek wrote: >

[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.

2016-11-30 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94 + /// \brief Adds a replacement that inserts \p Text at \p Loc. If this + /// insertion conflicts with an existing insertion (at the same position), + /// this will be inserted before

[PATCH] D27211: [clang-format] Implement comment reflowing

2016-12-01 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: unittests/Format/FormatTest.cpp:1787 +TEST_F(FormatTest, ReflowsComments) { + // Break a long line and reflow with the full next line. I think it is time that me moved a lot of the comment handling logic into a separ

[PATCH] D26909: [ClangFormat] Only insert #include into the #include block in the beginning of the file.

2016-12-02 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Format/Format.cpp:1521 +// tokens and returns an offset after the sequence. +unsigned getOffsetAfterTokenSequence( +StringRef FileName, StringRef Code, const FormatStyle &Style, I am somewhat hesitant to put more

[PATCH] D26909: [ClangFormat] Only insert #include into the #include block in the beginning of the file.

2016-12-02 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Comment at: unittests/Format/CleanupTest.cpp:898 + +TEST_F(CleanUpReplacementsTest, CanInsertAfterComment) { + std::string Code = "#include \"a.h\"\n"

[PATCH] D27383: Add a new clang-format style option ObjCBlockResetsIndent.

2016-12-03 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I think generally, this makes sense. Can you add tests in unittests/Format/FormatTest.cpp (probably next to TEST_F(FormatTest, FormatsBlocks) {..})? What's the behavior when there is more than one block? Also note that we have some requirements for additional options (m

[PATCH] D27615: [clang-format] calculate MaxInsertOffset in the original code correctly.

2016-12-09 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D27615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier

2016-12-13 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:297 case tok::kw_default: + if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) { +if (!SwitchLabelEncountered && Same as below.

[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier

2016-12-13 Thread Daniel Jasper via Phabricator via cfe-commits
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/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

2016-12-16 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. I agree that fallback-style should only be used when there is no .clang-format file. If we find one, and it doesn't parse correctly, we should neither use the fallback style nor scan in higher-level directories (not sure whether we currently do that). https://reviews.

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

2016-12-16 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Yes.. return non-zero seems right. This is an error condition. https://reviews.llvm.org/D27440 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28207: Add second fast path for DiagnosticsEngine::GetDiagStatePointForLoc

2017-01-01 Thread Daniel Jasper via Phabricator via cfe-commits
djasper created this revision. djasper added a reviewer: rsmith. djasper added a subscriber: cfe-commits. Herald added a subscriber: sanjoy. In many translation units I have tested, many of the calls to DiagnosticsEngine::GetDiagStatePointForLoc are for source locations before the first diag sta

[PATCH] D28208: Remove isIgnored()-test that is more expensive than the analysis behind it

2017-01-01 Thread Daniel Jasper via Phabricator via cfe-commits
djasper created this revision. djasper added a reviewer: rsmith. djasper added a subscriber: cfe-commits. In many translation units I have tried, the calls to isIgnored() removed in this patch are more expensive than doing the analysis that is behind it. The speed-up in translation units I have

[PATCH] D28218: Small optimizations for SourceManager::getFileID()

2017-01-02 Thread Daniel Jasper via Phabricator via cfe-commits
djasper created this revision. djasper added a reviewer: rsmith. djasper added a subscriber: cfe-commits. Specifically: - Separate one-entry cache for loaded and local files - Use bound that can be deduced from that cache for LessIndex - Address FIXME to use a faster alternative to isOffsetInFile

[PATCH] D28207: Add second fast path for DiagnosticsEngine::GetDiagStatePointForLoc

2017-01-03 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Basic/Diagnostic.cpp:179 + + // 2nd most frequent case: L is before the first diag state change. + FullSourceLoc FirstStateChangePos = DiagStatePoints[1].Loc; rsmith wrote: > It's surprising to me that this would b

[PATCH] D28207: Add second fast path for DiagnosticsEngine::GetDiagStatePointForLoc

2017-01-04 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Basic/Diagnostic.cpp:179 + + // 2nd most frequent case: L is before the first diag state change. + FullSourceLoc FirstStateChangePos = DiagStatePoints[1].Loc; rsmith wrote: > djasper wrote: > > rsmith wrote: > > >

[PATCH] D28208: Remove isIgnored()-test that is more expensive than the analysis behind it

2017-01-09 Thread Daniel Jasper via Phabricator via cfe-commits
djasper closed this revision. djasper added a comment. Landed as r290842. https://reviews.llvm.org/D28208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27651: [clang-format] Even with AlignConsecutiveDeclarations, PointerAlignment: Right should keep *s and &s to the right

2017-01-09 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. Just some nits. Thanks for working on this! Comment at: lib/Format/WhitespaceManager.cpp:158 +static bool IsPointerOrReference(tok::TokenKind Kind) { + return Kind == tok::star || Kind == tok::amp || Kind == tok::ampam

[PATCH] D28218: Small optimizations for SourceManager::getFileID()

2017-01-12 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added a comment. Ping? https://reviews.llvm.org/D28218 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

2017-01-16 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: Format/FormatToken.h:148 + /// \brief Whether the token is the final token in the identifier of a PP + // macro. This will be either 1) the identifier token following the 'define' This adds a lot of code, runtime and

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

2017-01-17 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/Format.cpp:1906 - // FIXME: If FallbackStyle is explicitly "none", format is disabled. - if (!getPredefinedStyle(FallbackStyle, Style.Languag

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

2017-10-18 Thread Daniel Jasper via Phabricator via cfe-commits
djasper added inline comments. Comment at: lib/Format/UsingDeclarationsSorter.cpp:79 const SourceManager &SourceMgr, tooling::Replacements *Fixes) { - SmallVector SortedUsingDeclarations( - UsingDeclarations->begin(), UsingDeclarations->end()); - std::stable_sort(Sort

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

2017-10-18 Thread Daniel Jasper via Phabricator via cfe-commits
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. LG. https://reviews.llvm.org/D39024 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

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

2017-11-01 Thread Daniel Jasper via Phabricator via cfe-commits
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 = AnnotatedLines.size()

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

2017-11-03 Thread Daniel Jasper via Phabricator via cfe-commits
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

2017-11-06 Thread Daniel Jasper via Phabricator via cfe-commits
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

2017-11-07 Thread Daniel Jasper via Phabricator via cfe-commits
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

2017-11-09 Thread Daniel Jasper via Phabricator via cfe-commits
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: http://starship.python.net/crew/mwh

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

2017-11-10 Thread Daniel Jasper via Phabricator via cfe-commits
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] D39478: [clang-format] Handle leading comments in using declaration

2017-11-10 Thread Daniel Jasper via Phabricator via cfe-commits
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] D40178: clang-format: [JS] remove trailing lines in arrow functions.

2017-11-17 Thread Daniel Jasper via Phabricator via cfe-commits
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 http://lists.l

<    1   2   3   4   5   >