[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 suspicious to me. State.Line->MustBeDeclaration is either true or false meaning that AllowAllParametersOfDeclarationOnNextLine or AllowAllArgumentsOnNextLine can always affect the behavior here, leading to BreakBeforeParameter to be set to true, even if we are in the case for PreviousIsBreakingCtorInitializerColon being true. So, my guess would be that if you set one of AllowAllArgumentsOnNextLine and AllowAllParametersOfDeclarationOnNextLine to false, then AllowAllConstructorInitializersOnNextLine doesn't have an effect anymore. Please verify, and if this is true, please fix and add tests. I think this might be easier to understand if you pulled the one if statement apart. https://reviews.llvm.org/D40988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/listinfo/cfe-commits
[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 comparison can have substantial cost. Here, specifically: - Make this more generic. This currently addresses a very specific use case (comment broken), whereas the action is probably always going to be the same (enforce break). I think we should call this "NoContinuation" or "NeedsNewline" or something. - This seems always to only relate to the previous token. So it can be always reset when the state is moved further. - As this always refers to the previous token, this can probably live in LineState instead of ParenState. That way, it has less runtime/space overhead. https://reviews.llvm.org/D37695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/listinfo/cfe-commits
[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(SortedUsingDeclarations.begin(),
- SortedUsingDeclarations.end());
- for (size_t I = 0, E = UsingDeclarations->size(); I < E; ++I) {
-if ((*UsingDeclarations)[I].Line == SortedUsingDeclarations[I].Line)
- continue;
-auto Begin = (*UsingDeclarations)[I].Line->First->Tok.getLocation();
-auto End = (*UsingDeclarations)[I].Line->Last->Tok.getEndLoc();
-auto SortedBegin =
-SortedUsingDeclarations[I].Line->First->Tok.getLocation();
-auto SortedEnd = SortedUsingDeclarations[I].Line->Last->Tok.getEndLoc();
-StringRef Text(SourceMgr.getCharacterData(SortedBegin),
- SourceMgr.getCharacterData(SortedEnd) -
- SourceMgr.getCharacterData(SortedBegin));
-DEBUG({
- StringRef OldText(SourceMgr.getCharacterData(Begin),
-SourceMgr.getCharacterData(End) -
-SourceMgr.getCharacterData(Begin));
- llvm::dbgs() << "Replacing '" << OldText << "' with '" << Text << "'\n";
-});
-auto Range = CharSourceRange::getCharRange(Begin, End);
-auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, Text));
-if (Err) {
- llvm::errs() << "Error while sorting using declarations: "
- << llvm::toString(std::move(Err)) << "\n";
+ if (*BlockAffected) {
+SmallVector SortedUsingDeclarations(
I'd prefer to iterator over UsingDeclarations here and see whether any of the
contained lines is Affected. Seems like a much more encapsulated way to do this
and prevents you from needing the awkward in-out parameter.
https://reviews.llvm.org/D39024
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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-commits
[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. How often does this happen for you? Why can't you just format the additional incorrect line? 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. 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.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 BreakBeforeTernaryOperators=false - I'd be ok with the suggestion for BreakBeforeTernaryOperators=true - I don't think the alignment of "?" and ":" (in the WhitespaceManager) should be always on. I think we'd need a flag for that part Manuel, Krasimir, WDYT? Repository: rC Clang https://reviews.llvm.org/D50078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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.org/cgi-bin/mailman/listinfo/cfe-commits
[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.org/cgi-bin/mailman/listinfo/cfe-commits
[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.org/cgi-bin/mailman/listinfo/cfe-commits
[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 options here are so straightforward now that I think I'd be ok if there isn't one. https://reviews.llvm.org/D32525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] 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.org/cgi-bin/mailman/listinfo/cfe-commits
[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 prefer to use std::max( .. ) (and we generally don't use c-style casts) Repository: rC Clang https://reviews.llvm.org/D43121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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.org/cgi-bin/mailman/listinfo/cfe-commits
[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
not fit on one line. I don't even know how I would format that, but it seems
important to know and not accidentally modify the behavior.
Repository:
rC Clang
https://reviews.llvm.org/D43180
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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.org/cgi-bin/mailman/listinfo/cfe-commits
[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/cfe-commits
[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 accept that or change their code to not
need it. I don't particularly care which of these options we go with, but I
would be really hesitant to introduce an style flag for it. This is so rare,
that almost no one needs this flag (or should need this flag). Thus, the cost
of the flag (discoverability of flags for users, increased maintenance cost,
etc.) strongly outweigh the benefit. IMO, for the same reason, this specific
issue will not become part of the style guide of projects.
If you want to change something, I'd vote for making clang fall back to this
behavior:
case A:
{
stuff();
}
moreStuff();
break;
}
i.e. not let it put the "{" on the same line as the "case ..." if there is a
trailing statement (other than "break;" maybe).
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/cfe-commits
[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,
> people will mostly accept the output of the formatter without question :
> therefore I think we should strive to have the formatter format things as
> "correctly" as possible. And looking at the existing options and various
> complex formatting algorithms implemented I think this reasoning has been
> applied to some extent :-)
> So I personnally would be willing to add some code/options even for such
> kind of corner cases; but then I am not the one maintaining the clang-format
> project.
I have no problem with making clang-format format things "correctly" and I am
happy to add code. But I think we are doing the average clang-format user a
disservice if we provide options for every such corner case. Let's settle on
one good-enough behavior and go with that for everything/everyone.
>> I don't particularly care which of these options we go with, but I would be
>> really hesitant to introduce an style flag for it. This is so rare, that
>> almost no one needs this flag (or should need this flag). Thus, the cost of
>> the flag (discoverability of flags for users, increased maintenance cost,
>> etc.) strongly outweigh the benefit.
>
> Any change will still require a new flag somewhere, since the currently
> implemented behavior is :
> 1- the documented way to format in Google style
> 2- the expected format in LLVM style, according to the clang-format unit test
>
> It should thus probably not be changed.
I don't think that's true for the alternative I am suggesting.
>> IMO, for the same reason, this specific issue will not become part of the
>> style guide of projects.
>
> I definitely agree on this, but it is actually part of some styles (e.g.
> Google's)
>
>> If you want to change something, I'd vote for making clang fall back to this
>> behavior:
>>
>> case A:
>> {
>> stuff();
>> }
>> moreStuff();
>> break;
>> }
>>
>>
>> i.e. not let it put the "{" on the same line as the "case ..." if there is a
>> trailing statement (other than "break;" maybe).
>
> I am fine with that formatting (though I did not implement it since it
> requires tweaking the code in more places, instead of essentially modifying a
> single function like I did).
As we can implement this without additional flags (it doesn't contradict any
style guide I know of), I think this would be strictly preferable.
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/cfe-commits
[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, depending on
> > your preference. If you do include curly braces they should be placed as
> > shown below.
> >
> > If not conditional on an enumerated value, switch statements should always
> > have a default case (in the case of an enumerated value, the compiler will
> > warn you if any values are not handled). If the default case should never
> > execute, simply assert:
> >
> > switch (var) {
> > case 0: { // 2 space indent
> > ... // 4 space indent
> > break;
> > }
> > case 1: {
> > ...
> > break;
> > }
> > default: {
> > assert(false);
> > }
> > }
>
> So IMHO we cannot just change the current (or default) behaviour.
My proposal does not contradict this style guide as the case in question is not
included in the example. It gives absolutely no guidance on how to format this
case.
If anything, you could argue that it tells the user never to have something
outside of the braces that make up a case statement (keep in mind that the
style guide does not only give formatting advise).
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/cfe-commits
[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.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 column limit
SomethingReallyLong =
{ { a, b, c },
{ a, b, c } };
Might not be as good as
| 80 column limit
SomethingReallyLong<
SomethingReallyLong> = {
{ a, b, c },
{ a, b, c }
};
in this mode.
Comment at: unittests/Format/FormatTest.cpp:6889
+ // Do not break between equal and opening brace.
+ FormatStyle avoidBreakingFirstArgument = getLLVMStyleWithColumns(43);
+ avoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
Upper camel case for variable names according to LLVM style.
Repository:
rC Clang
https://reviews.llvm.org/D43290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 llvm.org/PR35641". Comment at: unittests/Format/FormatTest.cpp:9457 + " int b;\n" + " int c;\n" + "}", Might be useful to use different types here to verify that alignment is actually happening, e.g. "int" and "unsigned". Repository: rC Clang https://reviews.llvm.org/D43303 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/listinfo/cfe-commits
[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.SpacesInContainerLiterals || + ((Style.Language == FormatStyle::LK_Proto || This is almost a duplicate of the one above. Can we pull out a function or lambda? Repository: rC Clang https://reviews.llvm.org/D43298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 braces you have this issue to some extend. But LLVM has several thousand switches, about a thousand with braces formatted this way and I am not aware of a single time this has caused a bug or even confusion. And to me (but this is obviously objective), your suggestions hide the structure of the code even more as they lead to a state where the closing brace is not aligned with the start of the line/statement that contains the opening braces. That looks like a bug to me more than anything else and I don't think there is another situation where clang-format would do that. 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/cfe-commits
[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 switch, let's put my
> function call here".
And then trying to format it with clang-format they'll be immediately thrown
off because clang-format gets the indentation wrong :).
But I don't want to argue this to death. I understand what you are saying. I
just don't think any of your suggested formats make this situation better.
> I am not saying it happens often, I am just saying it breaks indentation :
> i.e. that two nested blocks should not close at the same depth. Breaking such
> things makes code harder to read/understand, and when you don't properly get
> the code you can more easily make a mistake. Obviously it should be caught in
> testing, but it is not always.
>
> That said, I am not trying to say in any way that my approach is better. I
> think both `CaseBlockIndent = Block` or your variant (with the extra line
> break before opening brace; but applying it all the time) will indeed be
> consistent with the code and not break indentation; keeping the current
> behavior when `IndentCaseLabels = true` is also not a problem IMHO.
An extra thing to consider is that my approach is also consistent with having
something before this block:
case A:
{
f();
g();
}
h();
break;
case B:
f();
{
g();
}
h();
break;
>> And to me (but this is obviously objective), your suggestions hide the
>> structure of the code even more as they lead to a state where the closing
>> brace is not aligned with the start of the line/statement that contains the
>> opening braces. That looks like a bug to me more than anything else and I
>> don't think there is another situation where clang-format would do that.
>
> The exact same thing happens for functions blocks, class blocks and control
> statements, depending on BraceWrapping modes. Actually, IMO wrapping the
> opening brace should probably be done according to
> `BraceWrapping.AfterControlStatement` flag.
>
> // BraceWrapping.AfterControlStatement = false
> switch (x) {
> case 0: {
> foo();
> break;
> }
> }
> // BraceWrapping.AfterControlStatement = true
> switch (x)
> {
> case 0:
> {
> foo();
> break;
> }
> }
>
> But I agree the `CaseBlockIndent = ClosingBrace` mode is definitely not
> consistent with the code. I think it is clearer this way, but that is
> definitely my own subjective opinion: in this case, I accept to lose the
> extra indent for the sake of compactness and to somehow mimic the "regular"
> case blocks (e.g. which do not include an actual block), but that is indeed
> really my personnal way to read code.
I don't agree that that's the same thing. The closing brace is still neatly
aligned with the line of the opening brace (which happens to be just the
opening brace).
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/cfe-commits
[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.org/cgi-bin/mailman/listinfo/cfe-commits
[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. wrapping the template declarations? Will this stop at classes vs. functions? I generally have doubts that this option carries it's weight. Do you have a style guide that explicitly tells you to do this? Repository: rC Clang https://reviews.llvm.org/D42684 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 you actually care about? With Cpp11BracedListStyle=true or =false? > Generally, I think it is better to just rely on penalties, since it gives a > way to compare and weight each solution. Then each style can decide what > should break first: e.g. a style may also have a lower > `PenaltyBreakAssignment`, thus wrapping after the equal sign would be > expected... And with my reasoning, I think exactly the opposite. Penalties are nice, but if the behavior is generally unwanted, than it's very hard to predict in which situations it might still occur. Repository: rC Clang https://reviews.llvm.org/D43290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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) {
Here and in several other places: Variables should be named with upper camel
case
(https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+ Guesser.process();
+ if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
In LLVM, we generally don't add braces for single statement ifs.
Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+ if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+ }
Why not just return here?
Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
+struct GuessLanguageTestCase {
+ const char *const FileName;
IMO, this is a bit over-engineered. IIUC, this only calls a single free
standing function with two different parameters and expects a certain result.
You don't need this struct, a test fixture or parameterized tests for that.
Just write:
TEST(GuessLanguageTest, FileAndCode) {
EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
...
}
Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I think
that's actually good here. It makes the tests intuitively readable.
Repository:
rL LLVM
https://reviews.llvm.org/D43522
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 single statement ifs.
> Mmmm.. is this a hard requirement? I've personally been bitten so many times
> by adding statements after missing braces, I'd rather add them unless they're
> required to not be there by the style guide.
Yes. This is done as consistently as possible throughout LLVM and Clang and I'd
like to keep clang-format's codebase consistent.
clang-format itself makes it really hard to get bitten by missing braces :).
Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+ if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+ }
benhamilton wrote:
> djasper wrote:
> > Why not just return here?
> I don't like early returns in case an else sneaks in later:
>
> https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
But I would argue exactly the opposite. At this point, you have pretty uniquely
determined that this is ObjC (from this originally being viewed as LK_Cpp). Now
suppose you add additional logic before the return statement in line 2313: That
would make the state space that this function can have quite complex.
I would even go one step further and completely eliminate the variable
"result". That would be slightly less efficient, so maybe I'd be ok with:
const auto GuessFromFilename = getLanguageByFileName(FileName);
And then you can return this at the end or have early exits / other code paths
if you come up with different languages. Having both, a complex control flow
and state in local variables seems unnecessarily complex here.
Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
+struct GuessLanguageTestCase {
+ const char *const FileName;
benhamilton wrote:
> djasper wrote:
> > IMO, this is a bit over-engineered. IIUC, this only calls a single free
> > standing function with two different parameters and expects a certain
> > result. You don't need this struct, a test fixture or parameterized tests
> > for that. Just write:
> >
> > TEST(GuessLanguageTest, FileAndCode) {
> > EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
> > EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
> > ...
> > }
> >
> > Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I think
> > that's actually good here. It makes the tests intuitively readable.
> I hear you. I much prefer it when a single test tests a single issue, so
> failures are isolated to the actual change:
>
> https://elgaard.blog/2011/02/06/multiple-asserts-in-a-single-unit-test-method/
>
> If this isn't a hard requirement, I'd like to keep this the way it is.
It certainly makes sense for asserts, as a tests stops upon finding the first
assert. But these are expectations. Each failing expectation will be reported
individually, with a direct reference to the line in question and an easily
understandable error message.
I understand what you are saying but I think my proposal will actually make
test failures easier to diagnose and understand. Please do change it.
Repository:
rL LLVM
https://reviews.llvm.org/D43522
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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://reviews.llvm.org/D43673
Files:
lib/Lex/ModuleMap.cpp
test/Modules/Inputs/declare-use/h.h
Index: test/Modules/Inputs/declare-use/h.h
===
--- test/Modules/Inputs/declare-use/h.h
+++ test/Modules/Inputs/declare-use/h.h
@@ -1,7 +1,7 @@
#ifndef H_H
#define H_H
#include "c.h"
-#include "d.h" // expected-error {{does not depend on a module exporting}}
+#include "d.h" // expected-error {{module XH does not depend on a module
exporting}}
#include "h1.h"
const int h1 = aux_h*c*7*d;
#endif
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -475,7 +475,7 @@
// We have found a module, but we don't use it.
if (NotUsed) {
Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
return;
}
@@ -486,7 +486,7 @@
if (LangOpts.ModulesStrictDeclUse) {
Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
} else if (RequestingModule && RequestingModuleIsModuleInterface &&
LangOpts.isCompilingModule()) {
// Do not diagnose when we are not compiling a module.
Index: test/Modules/Inputs/declare-use/h.h
===
--- test/Modules/Inputs/declare-use/h.h
+++ test/Modules/Inputs/declare-use/h.h
@@ -1,7 +1,7 @@
#ifndef H_H
#define H_H
#include "c.h"
-#include "d.h" // expected-error {{does not depend on a module exporting}}
+#include "d.h" // expected-error {{module XH does not depend on a module exporting}}
#include "h1.h"
const int h1 = aux_h*c*7*d;
#endif
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -475,7 +475,7 @@
// We have found a module, but we don't use it.
if (NotUsed) {
Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
return;
}
@@ -486,7 +486,7 @@
if (LangOpts.ModulesStrictDeclUse) {
Diags.Report(FilenameLoc, diag::err_undeclared_use_of_module)
-<< RequestingModule->getFullModuleName() << Filename;
+<< RequestingModule->getTopLevelModule()->Name << Filename;
} else if (RequestingModule && RequestingModuleIsModuleInterface &&
LangOpts.isCompilingModule()) {
// Do not diagnose when we are not compiling a module.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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
+ verifyFormat("int f()\n"
+ "{\n"
+ " return 42;\n"
indent
https://reviews.llvm.org/D33447
___
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. 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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->TotalLength - Previous.TotalLength >
+ getColumnLimit(State) ||
Typz wrote:
> djasper wrote:
> > Typz wrote:
> > > djasper wrote:
> > > > Typz wrote:
> > > > > Typz wrote:
> > > > > > djasper wrote:
> > > > > > > Why can you drop the "+2" here?
> > > > > > >
> > > > > > > Also, I'd like to structure this so we have to duplicate less of
> > > > > > > the logic. But I am not really sure it's possible.
> > > > > > the +2 here was needed to keep identifiers aligned when breaking
> > > > > > after colon but before the command (e.g. the 4th combination, not
> > > > > > defined anymore):
> > > > > >
> > > > > > Foo() :
> > > > > > field(1)
> > > > > > , field(2)
> > > > > I can avoid some duplication like this,m but i am not convinced it
> > > > > helps :
> > > > >
> > > > > const FormatToken &ColonToken =
> > > > > Style.BreakConstructorInitializers !=
> > > > > FormatStyle::BCIS_AfterColon
> > > > > ? Current : Previous;
> > > > > if (ColonToken.is(TT_CtorInitializerColon) &&
> > > > > (State.Column + State.Line->Last->TotalLength -
> > > > > ColonToken.TotalLength +
> > > > >(Style.BreakConstructorInitializers !=
> > > > > FormatStyle::BCIS_AfterColon ? 2 : 0) >
> > > > >getColumnLimit(State) ||
> > > > >State.Stack.back().BreakBeforeParameter) &&
> > > > > (Style.AllowShortFunctionsOnASingleLine != FormatStyle::SFS_All
> > > > > ||
> > > > >Style.BreakConstructorInitializers !=
> > > > > FormatStyle::BCIS_BeforeColon ||
> > > > >Style.ColumnLimit != 0))
> > > > > return true;
> > > > >
> > > > > what do you think?
> > > > The +2 here has nothing todo with how the things are aligned. This is
> > > > about whether the entire constructor with initializer fits on one line.
> > > > Can you try out (or even add tests) for cases where the entire
> > > > constructor is 80 and 81 columns long?
> > > >
> > > > I think I like the condensed version a bit better, but yeah, it's not
> > > > beautiful either ;).
> > > My mistake, I read to quickly and talked about another +2 I removed from
> > > an earlier patch.
> > >
> > > As far as I understand it, this +2 accounts for the the "upcoming" space
> > > and colon, when checking if breaking _before_ the colon (e.g. before it
> > > was added to the line).
> > >
> > > Since this case is trying to break _after_ the colon, the space and colon
> > > have already been added to line (i.e. removed the column limit).
> > >
> > > The tests are already included (and I have just double-checked:
> > > `Constructor() : Initializer(FitsOnTheLine) {}` is indeed 45 characters) :
> > >
> > > verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}",
> > >getStyleWithColumns(Style, 45));
> > > verifyFormat("Constructor() :\n"
> > >"Initializer(FitsOnTheLine) {}",
> > >getStyleWithColumns(Style, 44));
> > > verifyFormat("Constructor() :\n"
> > >"Initializer(FitsOnTheLine) {}",
> > >getStyleWithColumns(Style, 43));
> > Ah, right. So as we are on the next token, State.Column will already
> > include the +2. However, I think we should change that and make this always:
> >
> > State.Column + State.Line->Last->TotalLength - Previous.TotalLength >
> > getColumnLimit(State)
> >
> > I think this should automatically add the "+2" or actually +1 should we go
> > forward with your patch to not have a space before the colon at some point.
> Seems to work indeed, looking much better!
> I had some trouble deciphering this when making my initial patch, and did not
> take the chance/risk to try to improve the 'regular' case.
Yeah, not surprising. This code isn't exactly nice or easy to understand or
well-commented :-(. Sorry about that.
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] 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
BraceWrapping.AfterFunction to true and AllowShortFunctionsOnASingleLine to
SFS_Empty and it would then format:
Constructor() {}
Constructor() //
: initializer(..)
{}
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/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 more special handling for macro-DSLs than strictly necessary.
https://reviews.llvm.org/D33440
___
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.
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(idx) {}
Moving the "{}" to the next line is only ok if the complete
function/constructor definition (up to and including the "}") does not fit on
one line.
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 can somewhat get out of that and you might be able to do the same thing here, but I am not entirely sure. In contrast, the added value is actually not very large. clang-format is merely not able to automatically fix something to your liking and it's very easy to make the code right and have clang-format keep it that way. https://reviews.llvm.org/D33440 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 meant 'conditional construct' : it happens when there are > parentheses around the expression, but it does not happen when these > parentheses are the parentheses from a `if (...)` Are you sure? From reading the code, it seems that this happens exactly after "=" and "return". What's the behavior for function calls? function(aaa // + b); Or for expressions with just parens? int a = (aa // + bb); What if doing this would violate the ContinuationIndentWidth? T t = a // && b; Comment at: lib/Format/ContinuationIndenter.cpp:759 +return State.Stack.back().Indent - Current.Tok.getLength() +- Current.SpacesRequiredBefore; if (State.Stack.back().Indent == State.FirstIndent && PreviousNonComment && Fix style. Comment at: lib/Format/ContinuationIndenter.cpp:949 + Previous->is(tok::kw_return))) + NewParenState.UnindentOperator = true; I am not yet convinced you need a new flag in ParenState here. I guess you need it because the operators can have varying length and so you cannot just change LastSpace here? https://reviews.llvm.org/D32478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 // == // && c; Where the syntactic structure is lost entirely. On top of that it has runtime downsides for all clang-format users because ParenState gets larger and more costly compare. As such, I am against moving forward with this. Can you remind me again, which coding style suggests this format? Comment at: unittests/Format/FormatTest.cpp:2619 + "sizeof(int16_t) // DWARF ARange version number\n" + " + sizeof(int32_t) // Offset of CU in the .debug_info section\n" + " + sizeof(int8_t) // Pointer Size (in bytes)\n" I think this is wrong and we should not indent like this. https://reviews.llvm.org/D32478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 many > > cases where it simply doesn't make sense. And then you have cases like this: > > > > bool = aa // > > == // > > && c; > > > > > > Where the syntactic structure is lost entirely. > > > It is not lost, extra indent for 'virtual' parenthesis is still there: > > bool a = aa // > == // > && c; Ah, right, I was thinking about something else (commas where we don't add extra indentation. Anyhow, I don't think what you are writing is what clang-format produces. How could it indent relative to the "&&" when placing the "=="? It doesn't know how far to unindent at that point, I think. > > >> On top of that it has runtime downsides for all clang-format users because >> ParenState gets larger and more costly compare. As such, I am against moving >> forward with this. Can you remind me again, which coding style suggests this >> format? > > This is just a single extra bit (and there are still less than 16 such bits), > so it does change the size of ParenState. As for the compare cost, I think it > is within reach of the compiler's optimization, but it may indeed have a > slight impact. https://reviews.llvm.org/D32478 ___ 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. 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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 > highest precedence is aligned with the equal sign. So, it's formatting like this? bool a = aa // == // && c; auto a = aa// == // + c; While that's interesting and I'd like to figure out how it actually works, I also think it's really weird to indent the inner "==" differently based on the kind of operator around it. The existing way to format these operators is nice and consistent here with always adding a multiple of four spaces. https://reviews.llvm.org/D32478 ___ 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.
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 relying on.
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 operators is not affected: the indentation is done > as "usual", then they are unindented by the operator width to keep the > alignment... Ah damn. Didn't think about the precedences. What I wanted was for the highest level to have a one char operator, e.g. bool a = aa // << // | c; However, you example is also interesting 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 many > > cases where it simply doesn't make sense. And then you have cases like this: > > > > bool = aa // > > == // > > && c; > > > > > > Where the syntactic structure is lost entirely. > > > It is not lost, extra indent for 'virtual' parenthesis is still there: > > bool a = aa // > == // > && c; > > > > On top of that it has runtime downsides for all clang-format users because > > ParenState gets larger and more costly compare. As such, I am against > > moving forward with this. Can you remind me again, which coding style > > suggests this format? > > This is just a single extra bit (and there are still less than 16 such bits), > so it does change the size of ParenState. As for the compare cost, I think it > is within reach of the compiler's optimization, but it may indeed have a > slight impact. I would still be interested in a coding style that recommends this format. https://reviews.llvm.org/D32478 ___ 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. 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 also ok with adding another value into BraceWrapping (which you suggested at some point, I think). https://reviews.llvm.org/D33447 ___ 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 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 comment.
Comment at: include/clang/Format/Format.h:654
+///
+bool AllowEmptyFunctionBody;
/// \brief Wrap before ``catch``.
I think the name probably isn't very intuitive. Maybe invert it and call it
"SplitEmptyFunctionBody"?
Comment at: unittests/Format/FormatTest.cpp:6092
+ verifyFormat("int f()\n"
+ "{\n"
+ " return 42;\n"
indent. Here and elsewhere.
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 && Previous.isNot(tok::r_paren) && + !(State.Column <= NewLineColumn && // Current.TokenText == "bind" && Style.Language == FormatStyle::LK_JavaScript)) Delete? https://reviews.llvm.org/D33640 ___ 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 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/listinfo/cfe-commits
[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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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-bin/mailman/listinfo/cfe-commits
[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.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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-commits
[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/mailman/listinfo/cfe-commits
[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().
Comment at: unittests/Format/FormatTest.cpp:1526
+ verifyFormat("const struct A a = {[0] = 1, [1] = 2};");
+ verifyFormat("const struct A a = {[1] = a,\n"
+ "[2] = b,\n"
Typz wrote:
> don't know why this test does not pass similarly to similar one with
> designated member access: in this case, clang-format puts each member in
> column.
I don't understand what you mean here.
https://reviews.llvm.org/D33491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/listinfo/cfe-commits
[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.llvm.org/D34351 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 header file. They are
implementation details. Either find a header/cpp-file in the lib/ directory to
place them in or just remove them for now. They are both still quite small and
called only twice each.
Comment at: unittests/Format/FormatTest.cpp:6530
+ verifyFormat("int f() {\n"
+ "}", MergeInlineOnly);
+
Missing line break.
Comment at: unittests/Format/FormatTest.cpp:6548
+ "{\n"
+ "}", MergeInlineOnly);
+ verifyFormat("class C {\n"
Missing line break. Generally use clang-format on the patches :).
https://reviews.llvm.org/D34399
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/listinfo/cfe-commits
[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/mailman/listinfo/cfe-commits
[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://reviews.llvm.org/D34395 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 this is any more or less strict than
the other version.
Comment at: unittests/Format/FormatTest.cpp:2781
+ verifyFormat("return (a)\n"
+ " // comment\n"
+ " + b;",
Comment seems to belong to "+ b" so should be aligned to it.
https://reviews.llvm.org/D32478
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/listinfo/cfe-commits
[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.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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"
+ "veryLongMethodParameter:(VeryLongParameterName)"
Set Style.ColumnLimit to something lower so that you don't have to wrap single
lines (for better test readability).
Repository:
rC Clang
https://reviews.llvm.org/D41195
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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. Which style do you actually care about? With
> > Cpp11BracedListStyle=true or =false?
>
> I use the `Cpp11BracedListStyle=false` style.
> However, I think the current behavior is wrong also when
> `Cpp11BracedListStyle=true`. Here the behavior in this case:
>
> Before :
>
> const std::unordered_map Something::MyHashTable =
> {{ "a", 0 },
>{ "b", 1 },
>{ "c", 2 }};
>
>
> After :
>
> const std::unordered_set Something::MyUnorderedSet = {
> { "a", 0 },
> { "b", 1 },
> { "c", 2 }};
"Before" is the desired behavior. I don't know whether other people have
extensively analyzed how to format all the various C++11 braced lists, but we
have at Google and think this is good. It is formalized here:
https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List_Format
We prefer breaking before "{" at the higher syntactic level or essentially
before the imaginary function call in place of the braced list.
>>> Generally, I think it is better to just rely on penalties, since it gives a
>>> way to compare and weight each solution. Then each style can decide what
>>> should break first: e.g. a style may also have a lower
>>> `PenaltyBreakAssignment`, thus wrapping after the equal sign would be
>>> expected...
>>
>> And with my reasoning, I think exactly the opposite. Penalties are nice, but
>> if the behavior is generally unwanted, than it's very hard to predict in
>> which situations it might still occur.
>
> Yet on the other hand enforcing too much can lead to cases where the
> optimizer fails to find a solution, and ends up simply not formatting the
> line: which is fine is the code is already formatted (but if there were the
> case we would not need the tool at all :-) )
> The beauty of penalties is that one can tweak the different penalties so
> that the behavior is "generally" what would be expected: definitely not
> predictable, but then there are always cases where the "regular" style does
> not work, and fallback solutions must be used... (for exemple, I would prefer
> never to never break after an assignment : yet sometimes it is needed...)
>
> I can change the patch to disallow breaking, but this would be a slightly
> more risky change, with possibly more side-effects; and i think that would
> not solve the issue when `Cpp11BracedListStyle=true`.
It is the better call here. I understand that people that set
Cpp11BracedListStyle to false want this behavior and I think it'll always be a
surprise when clang-format breaks before the "{". The chance of not coming up
with a formatting because of this is somewhat slim. It only adds an additional
two characters (we would also not break before the "="). It'd be really weird
to only have these exact two line length have a special behavior (slightly
shorter - everything fits on one line, slightly longer - a split between type
name and variable is necessary).
There is no issue for Cpp11BracedListStyle=true, the behavior is as desired.
Repository:
rC Clang
https://reviews.llvm.org/D43290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/FormatTest.cpp:6655
+ FormatStyle AvoidBreakingFirstArgument = getLLVMStyle();
+ AvoidBreakingFirstArgument.PenaltyBreakBeforeFirstCallParameter = 200;
+ verifyFormat("const std::unordered_map MyHashTable =\n"
How does this penalty influence the test?
Repository:
rC Clang
https://reviews.llvm.org/D43290
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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. > > - We have had about 10k clang-format users internally for several years. > > The semicolon issue comes up but really rarely and if it does, people > > happily fix their code not blaming clang-format. > > > > Unrelated, my point remains that setting BlockKind in TokenAnnotator is > > bad enough that I wouldn't want to do it for reaping this small benefit. > > And I can't see how you could easily achieve the same thing without doing > > that. > > > Just a question though. I there a reason brace matching (and other parts of > TokenAnnotations) are not performed before LineUnwrapping? That would > probably allow fixing this issue more cleanly (though I am not sure I would > have the time to actually perform this probably significant task)... I think this is just the way it has grown. And brace/paren matching is actually done in both places several times by now, I think :(. Fundamentally, the UnwrappedLineParser had the task of splitting a source file into lines and only implemented what it needed for that. The TokenAnnotator then did a much more elaborate analysis on each line to determine token types and such and distinguish what a "<" actually is (comparison vs. template opener). Having these two things be separate makes it slightly easier for error recovery and such as the state space they can be in is somewhat more limited. Repository: rC Clang https://reviews.llvm.org/D42729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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.org/D42684 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 do not want to move forward with this change. 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] 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, can you also run docs/tools/dump_format_style.py to
update the docs?
Comment at: unittests/Format/FormatTest.cpp:7539
+ verifyFormat("class Foo : public Bar {};", CtorInitializerStyle);
+ verifyFormat("Foo::Foo(): foo(1) {}", CtorInitializerStyle);
+ verifyFormat("for (auto a : b) {\n}", CtorInitializerStyle);
Can you add tests for the other values of BreakConstructorInitializers
(BCIS_BeforeColon, BCIS_BeforeComma)?
https://reviews.llvm.org/D32525
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] 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 BreakInheritanceListStyle {
+/// Break inheritance list before the colon and after the commas.
Update the docs with docs/tools/dump_format_style.py.
Repository:
rC Clang
https://reviews.llvm.org/D43015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 (
> const bbb & cc);
>
>
> instead of:
>
> template<> void
> (
> const bbb & cc);
>
>
> Then when this is fixed the penalty for breaking after the templates part is
> hardcoded to 10 (`prec::Level::Relational`), which was not always wrapping as
> expected (that is definitely subjective, but that is the beauty of
> penalties...)
Ah, I see. However, you are misunderstanding what the parameter is meant to do
(and I think what the name says). It is controlling whether we "always" break
before the template declaration (even if everything would fit on just one
line). Setting it to false, i.e. "not always" breaking, does not imply that
there is any particular situation in which we need to keep it on the same line.
I understand what you want to achieve, but I don't think it is related to
whether this is a function or a class declaration, i.e. clang-format also does:
template
class
: BB {};
although the template declaration would easily fit on the same line.
So this change does not seem like the right one to make in order to get the
options to be more intuitive and for you to get the behavior you want. I'll try
to think about how to achieve that. Do you have any ideas?
Repository:
rC Clang
https://reviews.llvm.org/D42684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 the indentation of the case labels:
switch (x) {
case 1: {
doSomething();
}
doSomethingElse();
break;
}
Fixing this is good no matter what.
And then the second question is whether this style should be used or not (with
IndentCaseLabels: false):
switch (x) {
case 1: {
doSomething();
}
}
Pro: Saves horizontal and vertical space.
Con: It's weird to have to braces in the same column.
I don't personally have an opinion here, but I'll check with a few LLVM
developers who work with LLVM style.
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/cfe-commits
[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()\n"
This is a useless test if it doesn't actually have multiple initializers
separated by a comma.
Comment at: unittests/Format/FormatTest.cpp:8971
+ verifyFormat("Fooo::Fooo()\n"
+ ": barr(1) {}", CtorInitializerStyle);
+ CtorInitializerStyle.BreakConstructorInitializers =
FormatStyle::BCIS_BeforeColon;
Has this been formatted by clang-format? I think it'd break after the comma.
Repository:
rC Clang
https://reviews.llvm.org/D32525
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] 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
circles.
Repository:
rC Clang
https://reviews.llvm.org/D32525
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] 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 > indenter only sees "short" class declarations unless breaking the template is > required because the name is too long). Not sure how that style flag is related to class declarations, but ok ;). > So just to be clear, are you saying the whole approach is not the right one, > or simply that the "names" of each modes are not? > For the name, maybe something like may be better: > > - `Never` > - `MultiLineDeclaration`, or maybe even `MultiLine` > - `Always` Ah, yeah, maybe it's just the name. "AlwaysBreak: Never" and "AlwaysBreak: Always" seem not very intuitive to interpret, though. How about just: No, Yes, MultiLine? Repository: rC Clang https://reviews.llvm.org/D42684 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 fit on one line if indented back to align with the open paren while it does fit on multiple lines if indented right of the comma. In all of LLVM/Clang, there seem to be only about 50 cases. How certain are you that people actually care? 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] 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 column, but he always cheats by removing the braces
for the last case label / default. You never need them. In any case he'd
strongly prefer the current behavior over adding an extra break and more
indentation.
In conclusion, I don't think we want to move forward with making clang-format do
switch (x) {
case 1:
{
doSomething();
}
}
even with IndentCaseLabels: false.
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/cfe-commits
[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 'default' behaviour for many people, > esp. when you don't yet use a formatter), and its seems our code base is > generally formatted like this. I think it is quite more frequent than 50 > times in whole LLVM/Clang, but I have no actual numbers; do you have a magic > tool to search for such specific "code pattern", or just run clang-format > with one option then the next and count the differences ? I just tweaked a search based on regular expressions. Fundamentally something like a line with an open paren and a comma that doesn't end in a comma gives a reasonable first approximation. But yes, first formatting with one option, then reformatting and looking at numbers of diffs would be interesting. And I would bet that even in your codebase diffs are few. Also double-checked with Richard Smith and he agrees that the current behavior is preferable. For comma and plus this doesn't seem overly important, but making it: aa(b + ccc * d); seems really bad to him as this suggests that we are adding both ccc and d. aa(b + ccc * d); seems clearer. And for consistency reasons, we should not treat those two cases differently. 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] 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/TokenAnnotator.cpp:357
+ return false;
+bool NamespaceAllowed;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
Replace lines 355-365 with:
bool IsUsingNamespace = AttrTok && AttrTok->startsSequence( ... );
if (IsUsingNamespace)
AttrTok = AttrTok->Next->Next->Next;
Comment at: lib/Format/TokenAnnotator.cpp:374
+ return false;
+return AttrTok->startsSequence(tok::r_square, tok::r_square);
+ }
Why not replace these three lines with:
return AttrTok && AttrTok->startsSequence(..);
Comment at: lib/Format/TokenAnnotator.cpp:400
+ next();
+ return true;
+}
This seems weird. I think if we return true, we should have consumed everything
up to the closing r_square.
Comment at: unittests/Format/FormatTest.cpp:12083
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
You are not adding any test (AFAICT) that tests that you have correctly set
TT_AttributeSpecifier or that you are making any formatting decisions based on
it. If you can't test it, remove that part of this patch.
Repository:
rC Clang
https://reviews.llvm.org/D43902
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 remember that in addition to (or instead of)
MightBeObjCForRangeLoop? That way, we wouldn't have to re-iterate over all the
tokens here, but could just set this directly.
Repository:
rC Clang
https://reviews.llvm.org/D43904
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {
This seems suspect. Does it have to be a numeric_constant?
Comment at: unittests/Format/FormatTest.cpp:12029
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+ EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
I am somewhat skeptical about all these tests now being solely about
guessLanguage. It might be the best choice for some of them, but also, there
might be other things we do to detect ObjC at some point and then all of these
tests become meaningless.
Does your change create a different formatting here?
Repository:
rC Clang
https://reviews.llvm.org/D43906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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))) {
benhamilton wrote:
> djasper wrote:
> > This seems suspect. Does it have to be a numeric_constant?
> Probably not, any constexpr would do, I suspect. What's the best way to parse
> that?
I think this is the same answer for both of your questions. If what you are
trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be enough to
look for whether there is a "(" after the ")" or even only after "(^)",
everything else is already correct IIUC? That would get you out of need to
parse the specifics here, which will be hard.
Or thinking about it another way. Previously, every "(^" would be parsed as an
ObjC block. There seems to be only a really rare corner case in which it isn't
(macros). Thus, I'd just try to detect that corner case. Instead you are
completely inverting the defaults (defaulting to "^" is not a block) and then
try to exactly parse ObjC where there might be many cases and edge cases that
you won't even think of now.
Repository:
rC Clang
https://reviews.llvm.org/D43906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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))) {
benhamilton wrote:
> benhamilton wrote:
> > djasper wrote:
> > > benhamilton wrote:
> > > > djasper wrote:
> > > > > This seems suspect. Does it have to be a numeric_constant?
> > > > Probably not, any constexpr would do, I suspect. What's the best way to
> > > > parse that?
> > > I think this is the same answer for both of your questions. If what you
> > > are trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be
> > > enough to look for whether there is a "(" after the ")" or even only
> > > after "(^)", everything else is already correct IIUC? That would get you
> > > out of need to parse the specifics here, which will be hard.
> > >
> > > Or thinking about it another way. Previously, every "(^" would be parsed
> > > as an ObjC block. There seems to be only a really rare corner case in
> > > which it isn't (macros). Thus, I'd just try to detect that corner case.
> > > Instead you are completely inverting the defaults (defaulting to "^" is
> > > not a block) and then try to exactly parse ObjC where there might be many
> > > cases and edge cases that you won't even think of now.
> > Hmm. Well, it's not just `FOO(^);` that isn't a block:
> >
> > ```
> > #define FOO(X) operator X
> >
> > SomeType FOO(^)(int x, const SomeType& y) { ... }
> > ```
> >
> > Obviously we can't get this perfect without a pre-processor, but it seems
> > like our best bet is to only assign mark `TT_ObjCBlockLParen` when we are
> > sure the syntax is a valid block type or block variable.
> I tried the suggestion to only treat `(^)(` as a block type, but it appears
> this is the primary place where we set `TT_ObjCBlockLParen`, so I think we
> really do need to handle the other cases here.
I don't follow your logic. I'd like you to slowly change this as opposed to
completely going the opposite way.
So currently, the only know real-live problem is "FOO(^);". So address this
somehow, but still default/error to recognizing too much stuff as a block.
Have you actually seen
SomeType FOO(^)(int x, const SomeType& y) { ... }
in real code?
Repository:
rC Clang
https://reviews.llvm.org/D43906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 yet another parenthesis counting. This function already iterates over all tokens until the closing paren. Just store a pointer to the caret here and mark it (or unmark it) when encountering the closing brace (line 271). There is already very similar logic there to set TT_FunctionTypeLParen (which is actually doing the exact same parsing now that I think of it). Repository: rC Clang https://reviews.llvm.org/D43906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
