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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

djasper added a comment. - Of course you find all sorts of errors while testing clang-format on a large-enough codebase. That doesn't mean that users run into them much. - We have had about 10k clang-format users internally for several years. The semicolon issue comes up but really rarely and if

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

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

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

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

[PATCH] D30399: clang-format: [JS] whitespace after async in arrow functions.

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2229 +if (Left.is(Keywords.kw_async) && Right.is(tok::l_paren) && +Right.MatchingParen && Right.MatchingParen->getNextNonComment() && +Right.MatchingParen->getNextNonComment()->is(TT_JsFa

[PATCH] D30452: Blacklist @mods and several other JSDoc tags from wrapping.

djasper added inline comments. Comment at: lib/Format/Format.cpp:627 +"(@(" +"const|" +"define|" I'd vote for making this "@\w*\ *{". We have seen incorrectly spelled version and such of this in the past. https://reviews.llvm.org/D30452

[PATCH] D30452: Blacklist @mods and several other JSDoc tags from wrapping.

djasper accepted this revision. djasper added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/Format.cpp:624 +// taze:, and @tag followed by { for a lot of JSDoc tags. +GoogleStyle.CommentPragmas = "(taze:|(@[A-Za-z_0-9-]*[ \\t]*{)

[PATCH] D30405: [clang-format] Add a new flag FixNamespaceEndComments to FormatStyle

djasper added inline comments. Comment at: include/clang/Format/Format.h:354 + /// fixes invalid existing ones. + bool FixNamespaceEndComments; + To be consistent with the clang-tidy check, just call this "FixNamespaceComments". After a change like this, you

[PATCH] D30405: [clang-format] Add a new flag FixNamespaceComments to FormatStyle

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

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added a comment. Could you please upload a diff with the entire file as context? That makes reviewing this easier. Comment at: docs/ClangFormatStyleOptions.rst:428 +**BreakBeforeInhertianceDelimiter** (``boolt``) + If ``true``, in the class inheritance expression cl

[PATCH] D30528: [clang-format] Use number of unwrapped lines for short namespace

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Please include the reasoning in the patch description, i.e. that otherwise clang-format might need to runs to add all the namespace comments. https://reviews.llvm.org/D30528

[PATCH] D30492: [clang-format] Allow all but the first string literal in a sequence to be put on a newline

djasper added a comment. As discussed offline, I think this solves the wrong problem. My guess is that breakProtrudingToken checks State.Stack.back().NoLinebreak, but I forget to make it also check NoLinebreakInOperand. https://reviews.llvm.org/D30492 ___

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

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added inline comments. Comment at: include/clang/Format/Format.h:309 + /// inheritance. + bool BreakInhertianceBeforeColonAndComma; + Hm. I am still not sure about this flag and it's name. Fundamentally, this is currently controlling two different th

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

djasper requested changes to this revision. djasper added a comment. This revision now requires changes to proceed. So, while it might be convenient to view this all in one file, a test here is not convenient for me (or presumably other clang-format developers) to work with. You can make a prett

[PATCH] D30532: Add examples to clang-format configuration

djasper added a comment. Hm. I don't actually know whether these examples are useful as is. You only present one setting of the value in most cases. What's interesting is actually how the flag changes the behavior. I mean in most cases, this can be derived from the example, but in those cases,

[PATCH] D30532: Add examples to clang-format configuration

djasper added a comment. Sure, then go ahead. If these examples would have helped you, that's one datapoint :). As for presenting the difference in options, that would be useful I think. So if you are up to also doing that, that'd be appreciated. For bool options it seems easiest to do somethin

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added inline comments. Comment at: include/clang/Format/Format.h:309 + /// inheritance. + bool BreakInhertianceBeforeColonAndComma; + Abpostelnicu wrote: > djasper wrote: > > Hm. I am still not sure about this flag and it's name. Fundamentally, this

[PATCH] D30532: Add examples to clang-format configuration

djasper added a comment. I agree, just generally we should aim for keeping these short. The example you gave might actually be reasonable to compare in two columns, i.e.: true: false: SomeClass::Constructor() vs. SomeClass::Constructor() : a(a),

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added a comment. Before `'`? Can you give an example? Repository: rL LLVM https://reviews.llvm.org/D30487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added a comment. Do you know whether that is intentional? The style guide isn't really conclusive. Repository: rL LLVM https://reviews.llvm.org/D30487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2486 Style.BreakConstructorInitializersBeforeComma && !Style.ConstructorInitializerAllOnOneLineOrOnePerLine) At any rate, I think this is what makes single-item ctor init lists

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added a comment. Hm. Unfortunately, this seems to have been implemented to support Webkit style and Webkit style is explicit about wanting this (https://webkit.org/code-style-guidelines/) :(. But maybe the solution to that is to add an extra flag like AlwaysWrapInitializerList. Really

[PATCH] D30532: Add examples to clang-format configuration

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

[PATCH] D30659: [clang-format] Make NamespaceEndCommentFixer add at most one comment

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Would probably be interesting to add these test cases: #if A namespace A { #else namespace B { #endif int i; int j; }//namespace A and: namespace A { int i; int j; #

[PATCH] D30688: [clang-format] Support namespaces ending in semicolon

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Minor nit, otherwise looks good. Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:152 +const FormatToken *EndCommentNextTok = EndCommentPrevTok->Next; +if (EndC

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

[PATCH] D30575: [clang-format] Look at NoLineBreak and NoLineBreakInOperand before breakProtrudingToken

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:852 + bool CanBreakProtrudingToken = + State.Stack.empty() || (!State.Stack.back().NoLineBreak && + !State.Stack.back().NoLineBreakInOperand); I thin

[PATCH] D30575: [clang-format] Look at NoLineBreak and NoLineBreakInOperand before breakProtrudingToken

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

[PATCH] D30734: Add more examples to clang-format configuration

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Please upload patches with full context or use arc (https://secure.phabricator.com/book/phabricator/article/arcanist/). Generally looks good, just minor comments. Comment

[PATCH] D30740: Remove a useless subsitution in doxygen2rst which was incorrectly replacing * by \*

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

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added a comment. I think the patch is fine, except for the name of the flag. It is not breaking inheritance ;). Maybe BreakBeforeInhertianceColonAndComma, but that's pretty long still. I think maybe we can shorten this to BreakBeforeInhertianceComma, as it never makes sense to break be

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. A few nits, otherwise looks good. Comment at: include/clang/Format/Format.h:426 + /// \brief If ``true``, in the class inheritance expression clang-format will + /// brea

[PATCH] D30777: Added `applyAtomicChanges` function.

djasper added inline comments. Comment at: include/clang/Tooling/Refactoring/AtomicChange.h:139 + // kNone: Don't format anything. + // kViolations: Format lines exceeding 80 columns. + enum FormatOption { kAll, kNone, kViolations }; Should probably be "exceed

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2666 return true; + if (Left.is(TT_InheritanceComma) && + Style.BreakBeforeInheritanceComma) Do these now fit on one line? Repository: rL LLVM https://reviews.llvm.org/D30487

[PATCH] D30874: clang-format: [JS] do not wrap after interface and type.

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

[PATCH] D30883: clang-format: [JS] do not wrap @see tags.

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

[PATCH] D30860: [clang-format] Add more examples and fix a bug in the py generation script

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Comment at: docs/tools/dump_format_style.py:67 def __str__(self): -return '* ``%s`` %s' % (self.name, doxygen2rst(self.comment)) +return '\n* ``%s``

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

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.

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.

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

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.

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.

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.

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.

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

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

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] D32298: [clang-format] Turn IncompleteFormat into a string

djasper added inline comments. Comment at: include/clang/Format/Format.h:1527 +/// non-recoverable syntax error. tooling::Replacements reformat(const FormatStyle &Style, StringRef Code, ArrayRef Ranges, This is a public interface

[PATCH] D32298: [clang-format] Replace IncompleteFormat by a struct with Line

djasper added inline comments. Comment at: include/clang/Format/Format.h:1519 + /// formatted due to a non-recoverable syntax error. + bool IncompleteFormat = false; + Maybe we should invert this and make this FormatComplete or something? Otherwise this is bou

[PATCH] D32298: [clang-format] Replace IncompleteFormat by a struct with Line

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

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

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:431 + + // Special case for AlignTokens: for all other alignment cases, + // the current sequence is ended when a comma or a scope change enyquist wrote: > djasp

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

<    1   2   3   4   5   >