djasper (Daniel Jasper)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 10 2012, 10:15 AM (301 w, 4 d)

Recent Activity

Fri, Apr 20

djasper added inline comments to D40988: Clang-format: add finer-grained options for putting all arguments on one line.
Fri, Apr 20, 12:47 AM

Thu, Apr 19

djasper accepted D45373: [clang-format] Don't remove empty lines before namespace endings.

Looks good.

Thu, Apr 19, 5:14 AM

Thu, Apr 12

djasper accepted D45526: [clang-format] Do not break after ObjC category open paren.

Looks good.

Thu, Apr 12, 7:58 AM
djasper added a comment to D45526: [clang-format] Do not break after ObjC category open paren.

I understand that, but the test example does not break after the closing paren. It breaks after the subsequent "<".

Thu, Apr 12, 6:58 AM
djasper added a comment to D45526: [clang-format] Do not break after ObjC category open paren.

Both patch and comment inside patch say "break after closing paren" and yet that's not what the test or example in the description actually do. Why is that?

Thu, Apr 12, 5:36 AM
djasper accepted D45004: [clang-format] Always indent wrapped Objective-C selector names.
Thu, Apr 12, 5:33 AM
djasper accepted D45521: [clang-format] Improve ObjC guessing heuristic by supporting all @keywords.

Looks good.

Thu, Apr 12, 5:31 AM
djasper accepted D45498: [clang-format] Don't insert space between ObjC class and lightweight generic.

Looks good.

Thu, Apr 12, 5:30 AM

Fri, Apr 6

djasper added a comment to D45004: [clang-format] Always indent wrapped Objective-C selector names.

Ok, you know the ObjC community much better than I do. If you think that adding the indentation for ObjC irrespective of the option works for most people, I am happy to go with it.

Fri, Apr 6, 4:45 AM

Thu, Apr 5

djasper added a comment to D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call.

Please read:

Thu, Apr 5, 11:35 PM
djasper added a comment to D45004: [clang-format] Always indent wrapped Objective-C selector names.

I'd go to great lengths to avoid adding new config options and so I don't think this would be a bad trade-off to make.

Thu, Apr 5, 9:54 AM
djasper accepted D45185: [clang-format] Support lightweight Objective-C generics.

Looks good, thank you!

Thu, Apr 5, 8:15 AM
djasper accepted D44996: [clang-format] Ensure ObjC selectors with 0 args are annotated correctly.
Thu, Apr 5, 2:37 AM
djasper added a comment to D45004: [clang-format] Always indent wrapped Objective-C selector names.

I still really believe that these config options do no make sense and are actively confusing.

Thu, Apr 5, 1:55 AM

Tue, Apr 3

djasper added inline comments to D45185: [clang-format] Support lightweight Objective-C generics.
Tue, Apr 3, 11:44 PM

Mon, Apr 2

djasper accepted D45169: [clang-format/ObjC] Do not detect "[]" as ObjC method expression.

Looks good. I like option 2 :).

Mon, Apr 2, 10:25 PM
djasper accepted D45168: [clang-format/ObjC] Do not insert space after opening brace of ObjC dict literal.

Looks good.

Mon, Apr 2, 10:25 PM

Fri, Mar 30

djasper accepted D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames.

Looks good.

Fri, Mar 30, 8:03 AM
djasper added inline comments to D44996: [clang-format] Ensure ObjC selectors with 0 args are annotated correctly.
Fri, Mar 30, 8:02 AM

Thu, Mar 29

djasper added inline comments to D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames.
Thu, Mar 29, 8:54 AM
djasper added a comment to D45004: [clang-format] Always indent wrapped Objective-C selector names.

Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent."

Thu, Mar 29, 8:44 AM
djasper added inline comments to D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames.
Thu, Mar 29, 5:27 AM
djasper added a reviewer for D45004: [clang-format] Always indent wrapped Objective-C selector names: thakis.
Thu, Mar 29, 5:14 AM
djasper added a comment to D45004: [clang-format] Always indent wrapped Objective-C selector names.

Do we have a public style guide that explicitly says to do this?
That's a requirement for new style options as per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.

Thu, Mar 29, 5:14 AM

Tue, Mar 27

djasper accepted D44816: [clang-format] Do not insert space before closing brace in ObjC dict literal.

Yeah, it's one of these things where neither way would be totally intuitive to everyone.

Tue, Mar 27, 7:55 AM
djasper accepted D44831: [clang-format] Refine ObjC guesser to handle child lines of child lines.

Looks good.

Tue, Mar 27, 7:54 AM
djasper accepted D44816: [clang-format] Do not insert space before closing brace in ObjC dict literal.

Generally looks good, one minor simplification.

Tue, Mar 27, 5:27 AM
djasper added inline comments to D44831: [clang-format] Refine ObjC guesser to handle child lines of child lines.
Tue, Mar 27, 2:09 AM

Mon, Mar 26

djasper added inline comments to D44831: [clang-format] Refine ObjC guesser to handle child lines of child lines.
Mon, Mar 26, 1:33 AM

Mar 23 2018

djasper added inline comments to D44790: [clang-format] Fix ObjC style guesser to also iterate over child lines.
Mar 23 2018, 5:48 AM

Mar 22 2018

djasper accepted D44790: [clang-format] Fix ObjC style guesser to also iterate over child lines.

Looks good, thank you!

Mar 22 2018, 10:29 AM
djasper accepted D42684: clang-format: Allow optimizer to break template declaration..

Some last comments, but basically looks good.

Mar 22 2018, 9:33 AM
djasper accepted D43290: clang-format: tweak formatting of variable initialization blocks.

Generally looks good.

Mar 22 2018, 8:04 AM
djasper committed rL328201: clang-format: Narrow down raw string literal line break exception..
clang-format: Narrow down raw string literal line break exception.
Mar 22 2018, 7:46 AM
djasper committed rC328201: clang-format: Narrow down raw string literal line break exception..
clang-format: Narrow down raw string literal line break exception.
Mar 22 2018, 7:46 AM
djasper committed rL328200: clang-format: Fix SpacesInParentheses with fully qualified names..
clang-format: Fix SpacesInParentheses with fully qualified names.
Mar 22 2018, 7:33 AM
djasper committed rC328200: clang-format: Fix SpacesInParentheses with fully qualified names..
clang-format: Fix SpacesInParentheses with fully qualified names.
Mar 22 2018, 7:33 AM

Mar 21 2018

djasper accepted D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic.

Looks good.

Mar 21 2018, 8:06 AM
djasper accepted D44692: [clang-format] Don't insert space between r_paren and 'new' in ObjC decl.

Looks good.

Mar 21 2018, 8:05 AM
djasper accepted D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set.

Please generally upload diffs with the full file as context so that Phabricator can properly expand the context where necessary.

Mar 21 2018, 5:27 AM

Mar 20 2018

djasper added inline comments to D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic.
Mar 20 2018, 8:37 AM
djasper added inline comments to D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic.
Mar 20 2018, 2:40 AM
djasper added a comment to D44634: [clang-format] Detect Objective-C for #import <Foundation/Foundation.h>.

I'd really like to not parse include/import statements this way. Can you send me examples of headers where we fail to recognize them as ObjC and this matters (happy for you to send me examples offline).

Mar 20 2018, 2:35 AM

Mar 12 2018

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

Ok, looks good.

Mar 12 2018, 8:34 AM
djasper committed rL327255: clang-format: Properly handle implicit string concatenation in text protos.
clang-format: Properly handle implicit string concatenation in text protos
Mar 12 2018, 3:35 AM
djasper committed rC327255: clang-format: Properly handle implicit string concatenation in text protos.
clang-format: Properly handle implicit string concatenation in text protos
Mar 12 2018, 3:35 AM
djasper committed rC327253: Don't re-format raw string literal contents when formatting is disable.
Don't re-format raw string literal contents when formatting is disable
Mar 12 2018, 3:15 AM
djasper committed rL327253: Don't re-format raw string literal contents when formatting is disable.
Don't re-format raw string literal contents when formatting is disable
Mar 12 2018, 3:14 AM

Mar 8 2018

djasper accepted D43906: [clang-format] Improve detection of Objective-C block types.

Nice.. Removed a lot of complexity :). Let's see whether this heuristic is good enough.

Mar 8 2018, 4:01 PM
djasper added inline comments to D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC.
Mar 8 2018, 3:56 PM

Mar 7 2018

djasper added inline comments to D43906: [clang-format] Improve detection of Objective-C block types.
Mar 7 2018, 4:48 AM

Mar 6 2018

djasper added a comment to D43906: [clang-format] Improve detection of Objective-C block types.

Right. So the difference is that for blocks, there is always a "(" after the "(^.....)". That should be easy to parse, no?

Mar 6 2018, 1:32 PM
djasper added inline comments to D43906: [clang-format] Improve detection of Objective-C block types.
Mar 6 2018, 11:34 AM
djasper added inline comments to D43906: [clang-format] Improve detection of Objective-C block types.
Mar 6 2018, 5:37 AM

Mar 5 2018

djasper added a comment to D43906: [clang-format] Improve detection of Objective-C block types.

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?

Mar 5 2018, 10:54 AM
djasper added inline comments to D43904: [clang-format] Improve detection of ObjC for-in statements.
Mar 5 2018, 10:45 AM
djasper added inline comments to D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC.
Mar 5 2018, 10:40 AM

Mar 2 2018

djasper added a comment to D42787: clang-format: do not add extra indent when wrapping last parameter.

We have talked about that and none of us agree.

Mar 2 2018, 7:09 AM
djasper added a comment to D42787: clang-format: do not add extra indent when wrapping last parameter.

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 ?

Mar 2 2018, 6:48 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

Got two responses so far.

Mar 2 2018, 6:41 AM

Mar 1 2018

djasper added a comment to D42787: clang-format: do not add extra indent when wrapping last parameter.

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?

Mar 1 2018, 7:12 AM
djasper added a comment to D42684: clang-format: Allow optimizer to break template declaration..

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

Mar 1 2018, 7:03 AM

Feb 28 2018

djasper accepted D32525: [clang-format] Add SpaceBeforeColon option.

Ah, I thought it was somehow possible to create:

Feb 28 2018, 10:00 AM
djasper added inline comments to D32525: [clang-format] Add SpaceBeforeColon option.
Feb 28 2018, 8:36 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

New options for this would not be acceptable IMO. Too much cost for too little benefit.

Feb 28 2018, 7:58 AM
djasper added a comment to D42684: clang-format: Allow optimizer to break template declaration..

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 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
    const bbbbbbbbbbbbbbbbbbb & cccccccccc);

instead of:

template<> void aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
    const bbbbbbbbbbbbbbbbbbb & cccccccccc);

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

Feb 28 2018, 7:52 AM
djasper added a comment to D43015: clang-format: Introduce BreakInheritanceList option.

If both this and https://reviews.llvm.org/D32525 are submitted, then we also need more tests for the combination of the two parameters.

Feb 28 2018, 7:37 AM
djasper added a comment to D32525: [clang-format] Add SpaceBeforeColon option.

I think this generally looks good, but needs a few more tests.

Feb 28 2018, 7:20 AM
djasper added a comment to D42787: clang-format: do not add extra indent when wrapping last parameter.

But you *do* want extra indentation in the case of:

Feb 28 2018, 7:15 AM
djasper added a comment to D42684: clang-format: Allow optimizer to break template declaration..

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)?

Feb 28 2018, 7:15 AM
djasper added a comment to D42729: clang-format: Fix formatting of function body followed by semicolon.
  • 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)...

Feb 28 2018, 7:08 AM

Feb 27 2018

djasper added inline comments to D43290: clang-format: tweak formatting of variable initialization blocks.
Feb 27 2018, 8:49 AM
djasper added a comment to D43290: clang-format: tweak formatting of variable initialization blocks.

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<std::string, int> Something::MyHashTable =
    {{ "aaaaaaaaaaaaaaaaaaaaa", 0 },
     { "bbbbbbbbbbbbbbbbbbbbb", 1 },
     { "ccccccccccccccccccccc", 2 }};

After :

const std::unordered_set<std::string> Something::MyUnorderedSet = {
    { "aaaaaaaaaaaaaaaaaaaaa", 0 },
    { "bbbbbbbbbbbbbbbbbbbbb", 1 },
    { "ccccccccccccccccccccc", 2 }};
Feb 27 2018, 6:30 AM

Feb 23 2018

djasper committed rL326024: Remove unused variable. We should be warning-free..
Remove unused variable. We should be warning-free.
Feb 23 2018, 11:00 PM
djasper committed rC326024: Remove unused variable. We should be warning-free..
Remove unused variable. We should be warning-free.
Feb 23 2018, 10:59 PM
djasper closed D43673: Make module use diagnostics refer to the top-level module.

Submitted as r326023.

Feb 23 2018, 10:56 PM
djasper committed rC326023: Make module use diagnostics refer to the top-level module.
Make module use diagnostics refer to the top-level module
Feb 23 2018, 10:56 PM
djasper committed rL326023: Make module use diagnostics refer to the top-level module.
Make module use diagnostics refer to the top-level module
Feb 23 2018, 10:56 PM
djasper created D43673: Make module use diagnostics refer to the top-level module.
Feb 23 2018, 5:25 AM

Feb 21 2018

djasper accepted D43598: [clang-format] Tidy up new API guessLanguage().

Thank you for doing these follow up changes!

Feb 21 2018, 3:38 PM
djasper added inline comments to D43522: [clang-format] New API guessLanguage().
Feb 21 2018, 3:20 PM
djasper added inline comments to D43522: [clang-format] New API guessLanguage().
Feb 21 2018, 9:45 AM

Feb 20 2018

djasper added a comment to D43290: clang-format: tweak formatting of variable initialization blocks.

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)

Feb 20 2018, 11:10 PM
djasper added a comment to D42684: clang-format: Allow optimizer to break template declaration..

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?

Feb 20 2018, 2:29 AM

Feb 19 2018

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

Looks good.

Feb 19 2018, 4:10 AM

Feb 15 2018

djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

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

Feb 15 2018, 8:12 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

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.

Feb 15 2018, 7:04 AM
djasper accepted D43298: [clang-format] Support repeated field lists in protos.

Looks good.

Feb 15 2018, 5:34 AM
djasper edited reviewers for D43312: [clang-format] fix handling of consecutive unary operators, added: krasimir; removed: djasper.
Feb 15 2018, 4:52 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

Yes, that's what I mean. What do you mean, the style is too error prone?

Feb 15 2018, 4:51 AM
djasper accepted D43303: [Format] Fix for bug 35641.

Thanks for the fix.

Feb 15 2018, 4:32 AM · Restricted Project

Feb 14 2018

djasper added a comment to D43290: clang-format: tweak formatting of variable initialization blocks.

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,

Feb 14 2018, 9:33 AM
djasper accepted D43294: [clang-format] Recognize percents as format specifiers in protos.

Ok.. I guess ;)

Feb 14 2018, 9:26 AM

Feb 13 2018

djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

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.

Feb 13 2018, 7:51 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

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.

Feb 13 2018, 6:36 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

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.

Feb 13 2018, 5:46 AM
djasper added a comment to D43183: clang-format: introduce `CaseBlockIndent` to control indent in switch.

Do you have a reference to style guides recommending any of this?

Feb 13 2018, 1:49 AM

Feb 12 2018

djasper accepted D43180: [clang-format] Support text proto extensions.

Cool, thanks.

Feb 12 2018, 11:28 PM
djasper added inline comments to D43180: [clang-format] Support text proto extensions.
Feb 12 2018, 7:05 AM

Feb 9 2018

djasper accepted D43121: clang-format: keep ObjC colon alignment with short object name.

Looks good.

Feb 9 2018, 6:30 AM