djasper accepted this revision.
djasper added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Format/TokenAnnotator.cpp:583
Contexts.back().ColonIsForRangeExpr = true;
+ // for async ( ...
+ if (CurrentToken->is(Keywords.kw_async
djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.
Are there cases where this makes a difference? If so, add a test. If not, add
something to the patch description to clarify.
Comment at: lib/Format/TokenAnnotator.cpp:2473
djasper added a comment.
Basically looks good, but please add a test case where the penalty is high show
that it changes behavior.
https://reviews.llvm.org/D32477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin
djasper added a comment.
The current behavior here is actually intended. If you'd like the other format,
we probably need to add a style option. What style guide are you basing this on?
Comment at: unittests/Format/FormatTest.cpp:2476
"bool value = a
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
djasper added inline comments.
Comment at: include/clang/Format/Format.h:153
+ /// \endcode
+ bool AllowSemicolonAfterNamespace;
+
While I am not entirely opposed to this feature, I think it should be a
separate patch.
Comment at: include/cl
djasper added a comment.
I have looked through the PDF you linked to, but I couldn't really find much
about formatting. I have found one instance of a constructor initializer list,
but there is no accompanying text saying that this is even intentional. Haven't
found a range-based for loop. As s
djasper added inline comments.
Comment at: include/clang/Format/Format.h:793
+ /// \endcode
+ bool DanglingParenthesis;
+
stringham wrote:
> djasper wrote:
> > I don't think this is a name that anyone will intuitively understand. I
> > understand that the nami
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"
-
djasper added a comment.
Yes, this definitely does not belong in the NamespaceEndCommentsFixer. It has
nothing to do with comments.
And I am also very skeptical about several things:
- Why start here? There are many places where semicolons could be cleaned up.
- If we add more of such cleanup f
djasper added inline comments.
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
Typz wrote:
> djasper wrote:
> > This is not bin packing at all. Maybe CompactNamespaces? Or
> > SingleLineNamespaces?
> >
> > Also, could
djasper added a comment.
I think we should just not do this for now. This addresses a very infrequent
case that's easy enough to fix manually. As such, it's not worth the added
complexity of clang-format and potential failures it might generate. Changing
non-whitespace/non-comment code is alway
djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.
nice
Comment at: lib/Format/ContinuationIndenter.cpp:590
1u, std::min(Current.NewlinesBefore, Style.MaxEmptyLinesToKeep + 1));
+bool InPPDirective =
+S
djasper added inline comments.
Comment at: include/clang/Format/Format.h:699
+ /// This option is **deprecated* and is retained for backwards compatibility.
bool BreakConstructorInitializersBeforeComma;
I don't think you need to keep this around. The YAML p
djasper added inline comments.
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
Typz wrote:
> djasper wrote:
> > Typz wrote:
> > > djasper wrote:
> > > > This is not bin packing at all. Maybe CompactNamespaces? Or
> > >
djasper added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:106
+ isLineComment(*Token) && Token->NewlinesBefore == 1 &&
+ Token->OriginalColumn == PreviousToken->OriginalColumn);
}
Any chance of moving this logic to d
djasper added a comment.
Hm, can't really remember what I meant by "my comment". Probably not important.
So, I still see two problems:
- I would not count the link you mentioned as a publicly accessible style guide.
- I don't think the naming and granularity of this option is right.
You specifi
djasper added a comment.
What I mean is that you should remove the CompactNamespace option and instead
let this be configured by an additional enum value in NamespaceIndentation.
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-comm
djasper accepted this revision.
djasper added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Format/UnwrappedLineParser.cpp:106
+ isLineComment(*Token) && Token->NewlinesBefore == 1 &&
+ Token->OriginalColumn == PreviousT
djasper added a comment.
That's what I meant by "The name NamespaceIndentation might then be a bit
confusing, but not sure whether it's worth changing it.".
I am honestly not sure. Let's get a third opinion.
If we add this additional option, I think we need to fix the behavior and make
what pe
djasper added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:196
+ FormatStyle::BCIS_AfterColonAndComma) &&
+ (State.Column + State.Line->Last->TotalLength - Previous.TotalLength >
+ getColumnLimit(State) ||
Typz wrote:
djasper added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:196
+ FormatStyle::BCIS_AfterColonAndComma) &&
+ (State.Column + State.Line->Last->TotalLength - Previous.TotalLength >
+ getColumnLimit(State) ||
Typz wrote:
djasper added a comment.
Don't C99 designated literals use "=" instead of ":"?
https://reviews.llvm.org/D32525
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
401 - 423 of 423 matches
Mail list logo
