[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style

djasper added a comment. Please don't add this as is. I don't usually run the file-based tests in my development workflow and suspect that I might be breaking this a lot. If you want something like this, please add it as unittest(s) in unittests/Format/... (either in a new file or in an

[PATCH] D30705: clang-format: [JS] allow breaking after non-null assertions.

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/TokenAnnotator.cpp:1056 +} else if (Current.is(tok::exclaim)) { + if (Style.Language == FormatStyle::LK_JavaScript) { +if

[PATCH] D30705: clang-format: [JS] allow breaking after non-null assertions.

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2292 return false; -// Postfix non-null assertion operator, as in `foo!.bar()`. -if (Right.is(tok::exclaim) && (Left.isOneOf(tok::identifier, tok::r_paren, -

[PATCH] D32170: Add a FixItHint for -Wmissing-prototypes to insert 'static '.

djasper added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11652 static bool ShouldWarnAboutMissingPrototype(const FunctionDecl *FD, - const FunctionDecl*& PossibleZeroParamPrototype) { + const FunctionDecl*&

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

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:413 + + while (Param && !Param->is(tok::l_paren)) { +if (!Param->is(tok::identifier) && !Param->is(tok::comma)) enyquist wrote: > djasper wrote: > > enyquist wrote: > > > djasper

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2267 + return true; +if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && +Left.Previous && Left.Previous->is(tok::kw_operator)) EricWF wrote: > djasper wrote: > > I

[PATCH] D34824: clang-format: add an option -verbose to list the files being processed

djasper added a comment. Generally upload diffs with full context to phabricator. That makes reviewing much easier. Comment at: test/Format/verbose.cpp:2 +// RUN: clang-format %s -verbose | FileCheck %s +// CHECK: Formatting + This seems like a pretty minimal

[PATCH] D35296: [clang-format] Keep level of comment before an empty line

djasper added a comment. I still think we should make this dependent on the current indentation. Why do you think this is better? https://reviews.llvm.org/D35296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines

djasper added inline comments. Comment at: lib/Format/Format.cpp:1958 LangOpts.DeclSpecKeyword = 1; // To get __declspec. + LangOpts.CoroutinesTS = 1; return LangOpts; Can we change this only if Style.isCpp()? We should probably do that for other things,

[PATCH] D34824: clang-format: add an option -verbose to list the files being processed

djasper added inline comments. Comment at: tools/clang-format/ClangFormat.cpp:377 break; case 1: Error = clang::format::format(FileNames[0]); sylvestre.ledru wrote: > djasper wrote: > > I think we should restructure the code to not have to duplicate

[PATCH] D35485: [clang-format] Fix comment levels between '}' and PPDirective

djasper added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:489 - nextToken(); // Munch the closing brace. + nextToken(InitialLevel); // Munch the closing brace. What happens if you instead change the Line->Level = InitialLevel; statement

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

djasper added a comment. So, there are two things in this patch: Statement macros and namespace macros. Lets break this out and handle them individually. They really aren't related that much. Statement macros: I think clang-format's handling here is good enough. clang-format does not insert

[PATCH] D35485: [clang-format] Fix comment levels between '}' and PPDirective

djasper added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:2378 ScopedLineState BlockState(*this, SwitchToPreprocessorLines); + if (InitialLevel) +Line->Level = *InitialLevel; What happens if we always set the Level to 0

[PATCH] D34824: clang-format: add an option -verbose to list the files being processed

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Yeah, improving the testsuite generally seems like a good idea. But unrelated to this patch. This looks good now. https://reviews.llvm.org/D34824

[PATCH] D35485: [clang-format] Fix comment levels between '}' and PPDirective

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

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

djasper created this revision. The case, I am particularly interested in is: #define A(x) \ ... \ if (...) { \ int SomeVariable = 1; \ ...; \ } Here, SomeVariable never leaves the scope of the macro and at

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

djasper added a comment. I'd be ok, with missing that case, but I am happy to hear more opinions. https://reviews.llvm.org/D35783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

djasper added a comment. Yes, your example would work, too. The specific use case I have is where we are shadowing global variables. https://reviews.llvm.org/D35783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35485: [clang-format] Fix comment levels between '}' and PPDirective

djasper added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:489 - nextToken(); // Munch the closing brace. + nextToken(InitialLevel); // Munch the closing brace. krasimir wrote: > djasper wrote: > > krasimir wrote: > > > djasper wrote: >

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

djasper added a comment. Like this? https://reviews.llvm.org/D35783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

djasper added a comment. I don't understand how my patch description says that. I am trying to be very explicit about that fact that it doesn't, see last paragraph. I have thought about what you are suggesting, but - A DeclContext doesn't have getEndLoc(). - The DeclContext NewDC here is

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

djasper updated this revision to Diff 107885. djasper added a comment. Updated to be a bit more strict (warn if declarations from the same context get shadowed). https://reviews.llvm.org/D35783 Files: lib/Sema/SemaDecl.cpp test/SemaCXX/warn-shadow.cpp Index: test/SemaCXX/warn-shadow.cpp

[PATCH] D35296: [clang-format] Keep level of comment before an empty line

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Some small comments, otherwise looks good. Comment at: lib/Format/TokenAnnotator.cpp:1706 +if (NextNonCommentLine && CommentLine) { + bool UpdateLevel =

[PATCH] D34824: clang-format: add an option -verbose to list the files being processed

djasper added a comment. Can you add a test? Tests for this kind of behavior of the clang-format command line go in test/Format/. Comment at: tools/clang-format/ClangFormat.cpp:380 +if (Verbose.getNumOccurrences() != 0) +outs() << "Formatting " << FileNames[0] <<

[PATCH] D34441: [clang-format] Support text proto messages

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:71 + const FormatStyle ) { + assert(LessTok.is(tok::less)); + return Style.Language == FormatStyle::LK_TextProto || Maybe rename to

[PATCH] D35015: [clang-format] Add space between a message field key and the opening bracket in proto messages

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

[PATCH] D34621: [clang-format] Support <>-style proto message fields

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

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

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

[PATCH] D34749: [clang-format] Fix parsing of msg{field}-style proto options

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

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

djasper added inline comments. Comment at: lib/Format/UnwrappedLineFormatter.cpp:723 FormatDecision LastFormat = Node->State.NextToken->Decision; if (LastFormat == FD_Unformatted || LastFormat == FD_Continue) +addNextStateToQueue(Penalty, Node,

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

djasper added a comment. Just make clang-format always do this. I don't think anyone is relying on the current behavior. https://reviews.llvm.org/D33932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32532: clang-format: [JS/Java] ignore Objective-C constructs in JS & Java.

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

[PATCH] D32531: clang-format: [JS] prevent wraps before class members.

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

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

djasper added a comment. What happens if the function call where this happens actually does not have multiple parameters but one parameter with many operands, e.g. changing your test case to: arg3 + is + quite + long + so + it + f(arguments << of << its <<

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

djasper added a comment. My point is though that even with only one argument, the BinPackArguments setting might lead to this bug. If not, that's good :). https://reviews.llvm.org/D32475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32590: clang-format: [JS] parse async function declarations.

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:1043 - // Parse function literal unless 'function' is the first token in a line - // in which case

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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] D32477: [clang-format] Allow customizing the penalty for breaking assignment

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

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

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 =

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

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] D33029: [clang-format] add option for dangling parenthesis

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

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

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:

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

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:

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

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

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

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

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

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

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

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

[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

[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

[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

[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

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

[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

[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

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

[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

[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

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

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

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

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

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

[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

[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

[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

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

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

[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

[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

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

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

<    1   2   3   4   5   >