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

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:760 (!Style.AllowAllParametersOfDeclarationOnNextLine && State.Line->MustBeDeclaration) || +(!Style.AllowAllArgumentsOnNextLine && This still looks suspiciou

[PATCH] 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 http://lists.llvm.org/cgi-bin/mailman/listi

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

[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 http://lists.llvm.org/cgi-bin/mailman/listi

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

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] D49580: [clang-format] Adding style option for absolute formatting

djasper added a comment. Could you explain what problem this is fixing? https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49580: [clang-format] Adding style option for absolute formatting

djasper added a comment. In my opinion, this only addresses one edge case where clang-format -lines output is not identical with a full reformatting. I believe they cannot all usefully be avoided. As such, I am unsure that this option carries its weight of making the implementation more complex

[PATCH] D49580: [clang-format] Adding style option for absolute formatting

djasper added a comment. Ok, so IIUC, understanding that @end effective ends a section much like "}" would address the currently observed problems? https://reviews.llvm.org/D49580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D50078: clang-format: support aligned nested conditionals formatting

djasper added a comment. I don't have very strong opinions here and I agree that we will need to improve the conditional formatting, especially as this kind of ternary-chaining is becoming more popular because of constexpr. My personal opinion(s): - I think this is a no-brainer for BreakBefore

[PATCH] D50132: [clang-format] Add some text proto functions to Google style

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

[PATCH] 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 http://lists.llvm.or

[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 http://lists.llvm.o

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

[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 http://lists.llvm.o

[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] 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 http://lists.llvm.o

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

[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 http://lists.llvm

[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 http://lists.llvm.org/cgi-bin/mailman/listinfo

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

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

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

[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 http://lists.ll

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

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

[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 http://lists.llvm.org/cgi-bin/mailman/l

[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) && + (Style.SpacesInContainerLitera

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

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

[PATCH] D43440: clang-format: [JS] fix `of` detection.

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

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

djasper added a comment. Please given an explanation of what you are trying to achieve with this change. Do you intend to set the penalty high so that clang-format does other things first before falling back to wrapping template declarations? Why treat separate declarations differently wrt. wra

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

djasper added a comment. In https://reviews.llvm.org/D43290#1008647, @Typz wrote: > Tweaking the penalty handling would still be needed in any case, since the > problem happens also when Cpp11BracedListStyle is true (though the effect is > more subtle) I don't understand this. Which style do

[PATCH] D43522: [clang-format] New API guessLanguage()

djasper added inline comments. Comment at: cfe/trunk/lib/Format/Format.cpp:2298 +FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) { + FormatStyle::LanguageKind result = getLanguageByFileName(FileName); + if (result == FormatStyle::LK_Cpp) { --

[PATCH] D43522: [clang-format] New API guessLanguage()

djasper added inline comments. Comment at: cfe/trunk/lib/Format/Format.cpp:2308 + Guesser.process(); + if (Guesser.isObjC()) { +result = FormatStyle::LK_ObjC; benhamilton wrote: > djasper wrote: > > In LLVM, we generally don't add braces for sin

[PATCH] D43598: [clang-format] Tidy up new API guessLanguage()

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Thank you for doing these follow up changes! Repository: rC Clang https://reviews.llvm.org/D43598 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D43673: Make module use diagnostics refer to the top-level module

djasper created this revision. djasper added a reviewer: rsmith. All use declarations need to be directly placed in the top-level module anyway, knowing the submodule doesn't really help. The header that has the offending #include can easily be seen in the diagnostics source location. https://

[PATCH] D43673: Make module use diagnostics refer to the top-level module

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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] D33447: clang-format: add option to merge empty function body

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

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

[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 http://lists.llvm.org/cgi-

[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 http://lists.

[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 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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[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 http://lists.llvm.org/cgi-bin

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

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

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

[PATCH] D34351: [clang-format] Simplify TT_SelectorName assignment logic

djasper added a comment. For the test introduced in https://reviews.llvm.org/rL262291, either of the changes causes a TT_SelectorName to become a TT_Unknown. So lets figure out how to make this a difference in observable formatting behavior instead of removing these checks. https://reviews.ll

[PATCH] D34399: clang-format: introduce InlineOnly short function style

djasper added inline comments. Comment at: include/clang/Format/Format.h:234 + bool allowEmptyFunctionsOnASingleLine() const { + return AllowShortFunctionsOnASingleLine >= ShortFunctionStyle::SFS_Empty; I'd prefer these functions not to be in the public h

[PATCH] D34399: clang-format: introduce InlineOnly short function style

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

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

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/D34238 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D34395: clang-format: add options to merge empty record body

djasper added a comment. Do you know of a style guide that would actually want to handle class, structs and unions differently? In most of Clang, they are handled as "records" and fundamentally, they are so alike that I'd hope that people always want the same behavior for all of them. https:/

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

djasper added a comment. I don't want to move forward with this patch. But adding Manuel as another reviewer to sanity-check. Comment at: include/clang/Format/Format.h:167 +/// \endcode +OAS_StrictAlign, + }; The name is not intuitive. I don't think t

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

djasper added inline comments. Comment at: include/clang/Format/Format.h:993 + /// inside ``IncludeCategories``. + bool IncludeRegexCaseInsensitive; + Do we really need a flag here? Shouldn't we just always do this? https://reviews.llvm.org/D33932

[PATCH] D34395: clang-format: add options to merge empty record body

djasper added a comment. Yes merge them into those two, please. I think we introduced the others because of some linux style, but generally lets try not to introduce options that people aren't going to use. https://reviews.llvm.org/D34395 ___ cfe-

[PATCH] D34395: clang-format: add options to merge empty record body

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

[PATCH] D34623: [clang-format] Add a test for associative map proto buffer fields

djasper added a comment. Can you create a more interesting test case where the map definition spans multiple lines? Possibly use qualified names for the field types. https://reviews.llvm.org/D34623 ___ cfe-commits mailing list cfe-commits@lists.llv

[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] D43290: clang-format: tweak formatting of variable initialization blocks

djasper added a comment. In https://reviews.llvm.org/D43290#1020537, @Typz wrote: > >> Tweaking the penalty handling would still be needed in any case, since the > >> problem happens also when Cpp11BracedListStyle is true (though the effect > >> is more subtle) > > > > I don't understand this.

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

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2183 return 0; +if (Left.Previous && Left.Previous->is(tok::equal) && +!Style.Cpp11BracedListStyle) Why is this necessary? Comment at: unittests/Format/

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

djasper added a comment. In https://reviews.llvm.org/D42729#1022069, @Typz wrote: > In https://reviews.llvm.org/D42729#994841, @djasper wrote: > > > - Of course you find all sorts of errors while testing clang-format on a > > large-enough codebase. That doesn't mean that users run into them much

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

djasper added a comment. I think it's possible that this is just a bug/oversight. But I don't fully understand the case where it is not behaving as you expect. Can you give me an example (config setting + code that's not formatted as you expect)? Repository: rC Clang https://reviews.llvm.or

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

djasper added a comment. But you *do* want extra indentation in the case of: function(a, b + cc); I understand you argument, but I don't agree at the moment. As is (without getting more feedback from others that clang-format is behaving unexpected here), I

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

djasper added a comment. I think this generally looks good, but needs a few more tests. Comment at: include/clang/Format/Format.h:1204 + /// \brief If ``false``, spaces will be removed before constructor initializer + /// colon. When this file is changed,

[PATCH] D43015: clang-format: Introduce BreakInheritanceList option

djasper added a comment. If both this and https://reviews.llvm.org/D32525 are submitted, then we also need more tests for the combination of the two parameters. Comment at: include/clang/Format/Format.h:852 + /// \brief Different ways to break inheritance list. + enum BreakI

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

djasper added a comment. In https://reviews.llvm.org/D42684#1022093, @Typz wrote: > The problem I have is really related to the current > `AlwaysBreakTemplateDeclarations` behavior, which does not apply to functions. > I set it to false, and I get this: > > template<> > void aaa

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

djasper added a comment. New options for this would not be acceptable IMO. Too much cost for too little benefit. I'd suggest to first make the change to fall back to the style with a regular block when there are statements other than break after the closing brace. That is always bad, no matter

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

djasper added inline comments. Comment at: unittests/Format/FormatTest.cpp:8969 + "barr(1) {}", CtorInitializerStyle); + CtorInitializerStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma; + verifyFormat("Fooo::Fooo(

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

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Ah, I thought it was somehow possible to create: Constructor(): aa() , bb() {}, but I guess clang-format always inserts a break there. Sorry for chasing you in circ

[PATCH] D42684: clang-format: Allow optimizer to break template declaration.

djasper added a comment. In https://reviews.llvm.org/D42684#1022219, @Typz wrote: > Indeed, seems to apply to classes as well. Maybe I was mislead by my testing, > where I did not get the case (possibly because we use > `ConstructorInitializerAllOnOneLineOrOnePerLine=true`, so the continuation

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

djasper added a comment. Are you sure that you are even addressing an important case? I have done some research on our codebase and this is something that happens incredibly rarely. The reason is that you have to have a very specific combination of line length, where the last parameter does not

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

djasper added a comment. Got two responses so far. 1. Having to closing braces in the same column is weird, but not that weird. Consider namespace n { void f() { ... } } 2. Richard Smith (code owner of Clang) says that he doesn't really like the two closing braces in the same co

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

djasper added a comment. In https://reviews.llvm.org/D42787#1025117, @Typz wrote: > If people don't care about this case, we might as well merge this :-) Just > kidding. > > The tweak matches both our expectation, the auto-indent behaviour of IDE (not > to be trusted, but still probably of 'def

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

djasper added a comment. We have talked about that and none of us agree. Repository: rC Clang https://reviews.llvm.org/D42787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:352 + return false; +if (!Tok.startsSequence(tok::l_square, tok::l_square)) + return false; Just join this if with the previous one. Comment at: lib/Format/T

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:288 +if (MightBeObjCForRangeLoop) { + FormatToken *ForInToken = Left; There can be only one ObjCForIn token in any for loop, right? If that's the case, can we just rememb

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

djasper added a comment. Would it be enough to only add the block type case? With the macro false positive, there won't be an open paren after the closing paren, right? Comment at: lib/Format/TokenAnnotator.cpp:155 + Next->startsSequence(tok::identifier, tok::l_squar

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:155 + Next->startsSequence(tok::identifier, tok::l_square, +tok::numeric_constant, tok::r_square, +tok::r_paren, tok::l_paren))) { --

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:155 + Next->startsSequence(tok::identifier, tok::l_square, +tok::numeric_constant, tok::r_square, +tok::r_paren, tok::l_paren))) { --

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

djasper added a comment. Right. So the difference is that for blocks, there is always a "(" after the "(^.)". That should be easy to parse, no? Repository: rC Clang https://reviews.llvm.org/D43906 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:152 + const FormatToken *Next = CurrentToken->getNextNonComment(); + int ParenDepth = 1; + // Handle nested parens in case we have an array of blocks with No. Don't implement

  1   2   3   4   5   >