djasper (Daniel Jasper)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

djasper added inline comments to D33447: clang-format: add option to merge empty function body.
Fri, May 26, 8:42 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

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

Fri, May 26, 7:13 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.
In D32478#765642, @Typz wrote:

Nop, it's formatted like this:

bool a = aaaaaa   //
          == bbbb //
      && ccccc;
 
bool a = aaaaaa //
      == bbbb   //
             + ccccc;

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

Fri, May 26, 7:00 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong:

Fri, May 26, 6:26 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.
In 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.

Fri, May 26, 6:20 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

I don't understand. WebKit style would not set AllowShortFunctionsOnASingleLine and so the behavior there wouldn't change, I presume?

Fri, May 26, 5:43 AM
djasper committed rL303974: Remove unnecessary double-assignment triggering -Wsequence-point..
Remove unnecessary double-assignment triggering -Wsequence-point.
Fri, May 26, 5:07 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.
In D32478#765548, @Typz 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 = aaaaaa //
    == bbbb //
    && ccccc;

Where the syntactic structure is lost entirely.

It is not lost, extra indent for 'virtual' parenthesis is still there:

bool a = aaaaaa   //
          == bbbb //
      && ccccc;
Fri, May 26, 5:05 AM
djasper added inline comments to D33560: [clang-format] Add option to specify explicit config file.
Fri, May 26, 5:00 AM
djasper edited reviewers for D33589: clang-format: consider not splitting tokens in optimization, added: alexfh; removed: djasper.
Fri, May 26, 4:46 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.

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:

Fri, May 26, 4:21 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.
In D32478#759347, @Typz 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 (...)

Fri, May 26, 1:00 AM

Wed, May 24

djasper added a comment to D33440: clang-format: properly handle Q_UNUSED and QT_REQUIRE_VERSION.

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.

Wed, May 24, 5:53 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

No, I don't think it should be done this way and neither Facebook nor Mozilla coding styles say you should.

Wed, May 24, 5:39 AM
djasper added a comment to D33440: clang-format: properly handle Q_UNUSED and QT_REQUIRE_VERSION.

I don't. Only if they start out to be on the same line. As long as I start with:

Wed, May 24, 5:35 AM
djasper added a comment to D33440: clang-format: properly handle Q_UNUSED and QT_REQUIRE_VERSION.

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?

Wed, May 24, 4:52 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

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

Wed, May 24, 4:50 AM
djasper accepted D32479: clang-format: Introduce BreakConstructorInitializers option.

Looks good. Thank you!

Wed, May 24, 3:21 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

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?

Wed, May 24, 3:19 AM
djasper added a comment to D33447: clang-format: add option to merge empty function body.

Does anything speak against making this behavior happen with AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra style option?

Wed, May 24, 2:56 AM

Tue, May 23

djasper added a comment to D32525: [clang-format] Add SpaceBeforeColon option.

Don't C99 designated literals use "=" instead of ":"?

Tue, May 23, 5:25 AM
djasper added inline comments to D32479: clang-format: Introduce BreakConstructorInitializers option.
Tue, May 23, 5:20 AM
djasper added inline comments to D32479: clang-format: Introduce BreakConstructorInitializers option.
Tue, May 23, 4:34 AM
djasper added a reviewer for D32480: clang-format: Add CompactNamespaces option: klimek.
Tue, May 23, 4:26 AM · Restricted Project
djasper added a comment to D32480: clang-format: Add CompactNamespaces option.

That's what I meant by "The name NamespaceIndentation might then be a bit confusing, but not sure whether it's worth changing it.".

Tue, May 23, 4:26 AM · Restricted Project

Mon, May 22

djasper accepted D33399: clang-format: [JS] avoid line breaks before unindented r_parens..
Mon, May 22, 7:46 AM
djasper accepted D33394: [clang-format] Keep trailing preprocessor line comments separate from the following section comments.
Mon, May 22, 3:02 AM
djasper added a comment to D32480: clang-format: Add CompactNamespaces option.

What I mean is that you should remove the CompactNamespace option and instead let this be configured by an additional enum value in NamespaceIndentation.

Mon, May 22, 2:44 AM · Restricted Project
djasper added a comment to D32525: [clang-format] Add SpaceBeforeColon option.

Hm, can't really remember what I meant by "my comment". Probably not important.

Mon, May 22, 2:42 AM
djasper added inline comments to D33394: [clang-format] Keep trailing preprocessor line comments separate from the following section comments.
Mon, May 22, 2:12 AM
djasper added inline comments to D32480: clang-format: Add CompactNamespaces option.
Mon, May 22, 2:07 AM · Restricted Project
djasper added inline comments to D32479: clang-format: Introduce BreakConstructorInitializers option.
Mon, May 22, 1:44 AM

Fri, May 19

djasper accepted D33351: [clang-format] Handle trailing comment sections in import statement lines.

nice

Fri, May 19, 3:41 AM
djasper added a comment to D33314: clang-format: Add option to remove semicolon at end of namespace.

I think we should just not do this for now. This addresses a very infrequent case that's easy enough to fix manually. As such, it's not worth the added complexity of clang-format and potential failures it might generate. Changing non-whitespace/non-comment code is always dangerous.

Fri, May 19, 12:50 AM

Thu, May 18

djasper accepted D32477: clang-format: Allow customizing the penalty for breaking assignment.
Thu, May 18, 11:59 PM
djasper added inline comments to D32480: clang-format: Add CompactNamespaces option.
Thu, May 18, 11:54 PM · Restricted Project
djasper added a comment to D33314: clang-format: Add option to remove semicolon at end of namespace.

Yes, this definitely does not belong in the NamespaceEndCommentsFixer. It has nothing to do with comments.

Thu, May 18, 11:31 PM
djasper accepted D33329: clang-format: [JS] for await, and fix a crash with for loops..
Thu, May 18, 2:23 PM
djasper committed rL303339: Revert r302781 and subsequent attempts to disable part of it..
Revert r302781 and subsequent attempts to disable part of it.
Thu, May 18, 2:45 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.

When you say "this doesn't happen in tests", do you mean this never happens when there are parentheses around the expression?

Thu, May 18, 12:12 AM
djasper removed a reviewer for D33282: clang-format: fix prefix for doxygen comments after member: djasper.
Thu, May 18, 12:00 AM

Wed, May 17

djasper removed a reviewer for D33285: clang-format: do not reflow bullet lists: djasper.
Wed, May 17, 1:38 PM
djasper added inline comments to D33029: [clang-format] add option for dangling parenthesis.
Wed, May 17, 6:14 AM · Restricted Project
djasper added a comment to D32525: [clang-format] Add SpaceBeforeColon option.

I have looked through the PDF you linked to, but I couldn't really find much about formatting. I have found one instance of a constructor initializer list, but there is no accompanying text saying that this is even intentional. Haven't found a range-based for loop. As such, I am not convinced that this option carries it's weight.

Wed, May 17, 6:09 AM
djasper added inline comments to D32480: clang-format: Add CompactNamespaces option.
Wed, May 17, 6:01 AM · Restricted Project
djasper added a comment to D32479: clang-format: Introduce BreakConstructorInitializers option.

Yes, turning that option into an enum seems like the better choice here.

Wed, May 17, 5:57 AM
djasper added a comment to D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set.

The current behavior here is actually intended. If you'd like the other format, we probably need to add a style option. What style guide are you basing this on?

Wed, May 17, 5:54 AM
djasper added a comment to D32477: clang-format: Allow customizing the penalty for breaking assignment.

Basically looks good, but please add a test case where the penalty is high show that it changes behavior.

Wed, May 17, 5:50 AM
djasper accepted D33238: [clang-format] Make NoLineBreakFormatter respect MustBreakBefore.

Are there cases where this makes a difference? If so, add a test. If not, add something to the patch description to clarify.

Wed, May 17, 5:41 AM

Mon, May 15

djasper accepted D33193: clang-format: [JS] for async loops..
Mon, May 15, 6:29 AM
djasper accepted D33023: clang-format: [JS] wrap params with trailing commas..
Mon, May 15, 4:21 AM
djasper added inline comments to D33023: clang-format: [JS] wrap params with trailing commas..
Mon, May 15, 2:41 AM
djasper added inline comments to D33023: clang-format: [JS] wrap params with trailing commas..
Mon, May 15, 1:42 AM
djasper added a comment to D33182: clang-format: [JS] fix non-null assertion operator recognition..

A test case might be useful.

Mon, May 15, 1:28 AM
djasper accepted D33182: clang-format: [JS] fix non-null assertion operator recognition..
Mon, May 15, 1:28 AM
djasper committed rL303037: Revert r302965 - [modules] When creating a declaration, cache its owning.
Revert r302965 - [modules] When creating a declaration, cache its owning
Mon, May 15, 1:04 AM

Sun, May 14

djasper committed rL303034: Add '#' to test regex that I forgot in r303025..
Add '#' to test regex that I forgot in r303025.
Sun, May 14, 10:11 PM
djasper committed rL303025: Fix two tests that weren't correctly copied..
Fix two tests that weren't correctly copied.
Sun, May 14, 3:21 PM

Fri, May 12

djasper added a comment to D33029: [clang-format] add option for dangling parenthesis.

Probably all of the examples from the original patch description and later comments should be turned into unit tests.

Fri, May 12, 11:14 PM · Restricted Project

Thu, May 11

djasper accepted D33113: clang-format: [JS] support non-null assertions after all identifiers..
Thu, May 11, 10:06 PM

Wed, May 10

djasper accepted D32825: [clang-format] Improve understanding of combined typedef+record declarations.

One nit, otherwise looks good.

Wed, May 10, 10:36 PM
djasper accepted D33006: clang-format: refine calculating brace types..

Looks good.

Wed, May 10, 6:32 AM

Tue, May 9

djasper added a comment to D33029: [clang-format] add option for dangling parenthesis.

Please read and address: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options

Tue, May 9, 10:40 PM · Restricted Project
djasper accepted D32997: clang-format: [JS] keep triple slash directives intact..

Looks good.

Tue, May 9, 5:56 AM

Mon, May 8

djasper closed D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions.
Mon, May 8, 8:21 AM
djasper accepted D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions.

Submitted as r302427.

Mon, May 8, 8:21 AM
djasper closed D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign.

Submitted as r302428.

Mon, May 8, 8:21 AM
djasper committed rL302428: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding.
[clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding
Mon, May 8, 8:21 AM
djasper committed rL302427: [clang-format] Don’t propagate AvoidBinPacking into argument.
[clang-format] Don’t propagate AvoidBinPacking into argument
Mon, May 8, 8:21 AM

Sat, May 6

djasper committed rL302352: Creating tags/google/stable/2017-05-06 from r302012.
Creating tags/google/stable/2017-05-06 from r302012
Sat, May 6, 10:19 AM
djasper committed rL302353: Updating branches/google/stable to r302012.
Updating branches/google/stable to r302012
Sat, May 6, 10:18 AM
djasper committed rL302342: Creating tags/google/stable/2017-05-06 from r302012.
Creating tags/google/stable/2017-05-06 from r302012
Sat, May 6, 9:43 AM
djasper committed rL302343: Updating branches/google/stable to r302012.
Updating branches/google/stable to r302012
Sat, May 6, 9:43 AM
djasper committed rL302351: Updating branches/google/stable to r302012.
Updating branches/google/stable to r302012
Sat, May 6, 9:42 AM
djasper committed rL302350: Creating tags/google/stable/2017-05-06 from r302012.
Creating tags/google/stable/2017-05-06 from r302012
Sat, May 6, 9:41 AM
djasper committed rL302349: Updating branches/google/stable to r302012.
Updating branches/google/stable to r302012
Sat, May 6, 9:27 AM
djasper committed rL302348: Creating tags/google/stable/2017-05-06 from r302012.
Creating tags/google/stable/2017-05-06 from r302012
Sat, May 6, 9:24 AM
djasper committed rL302341: Cleaning up stable branch.
Cleaning up stable branch
Sat, May 6, 9:23 AM
djasper committed rL302347: Updating branches/google/stable to r302012.
Updating branches/google/stable to r302012
Sat, May 6, 9:20 AM
djasper committed rL302346: Creating tags/google/stable/2017-05-06 from r302012.
Creating tags/google/stable/2017-05-06 from r302012
Sat, May 6, 9:18 AM
djasper committed rL302345: Updating branches/google/stable to r302012.
Updating branches/google/stable to r302012
Sat, May 6, 9:16 AM
djasper committed rL302344: Creating tags/google/stable/2017-05-06 from r302012.
Creating tags/google/stable/2017-05-06 from r302012
Sat, May 6, 9:12 AM

Fri, May 5

djasper committed rL302214: Initialize new member X86Operand::FrontendSize in all codepaths..
Initialize new member X86Operand::FrontendSize in all codepaths.
Fri, May 5, 12:44 AM

Thu, May 4

djasper accepted D32864: clang-format: [JS] exponentiation operator.

Thanks

Thu, May 4, 8:12 AM
djasper accepted D32864: clang-format: [JS] exponentiation operator.
Thu, May 4, 8:03 AM

Wed, May 3

djasper committed rL302012: Undo turning ExtBehavior into a bitfield..
Undo turning ExtBehavior into a bitfield.
Wed, May 3, 4:40 AM
djasper committed rL302009: Fix tests after speculatable intrinsics patch.
Fix tests after speculatable intrinsics patch
Wed, May 3, 3:17 AM
djasper committed rL302003: Silences gcc's -Wnarrowing..
Silences gcc's -Wnarrowing.
Wed, May 3, 1:01 AM
djasper committed rL302002: Revert r301986 (and subsequent r301987)..
Revert r301986 (and subsequent r301987).
Wed, May 3, 12:42 AM

Tue, May 2

djasper committed rL301963: Revert r301822 (and dependent r301825), which tried to improve the.
Revert r301822 (and dependent r301825), which tried to improve the
Tue, May 2, 12:34 PM
djasper committed rL301916: Revert r301735 (and subsequent r301786)..
Revert r301735 (and subsequent r301786).
Tue, May 2, 5:51 AM
djasper accepted D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign.

This is an edge case in actual C++. An escaped newline literally gets expanded to nothing. So what this reads is

Tue, May 2, 12:41 AM
djasper edited reviewers for D32429: Add capability to format the diff on save to clang-format's vim integration, added: klimek; removed: chandlerc, bkramer.
Tue, May 2, 12:00 AM

Mon, May 1

djasper added inline comments to D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign.
Mon, May 1, 11:43 PM

Apr 27 2017

djasper accepted D30742: add option -fallback-style=fail (fail if -style=file falls back).

Looks good.

Apr 27 2017, 6:15 AM · Restricted Project
djasper accepted D32590: clang-format: [JS] parse async function declarations..

Looks good.

Apr 27 2017, 6:14 AM
djasper added inline comments to D30742: add option -fallback-style=fail (fail if -style=file falls back).
Apr 27 2017, 4:28 AM · Restricted Project

Apr 26 2017

djasper added a comment to D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions.

My point is though that even with only one argument, the BinPackArguments setting might lead to this bug. If not, that's good :).

Apr 26 2017, 1:05 PM
djasper added a comment to D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions.

What happens if the function call where this happens actually does not have multiple parameters but one parameter with many operands, e.g. changing your test case to:

Apr 26 2017, 11:07 AM
djasper added a comment to D32429: Add capability to format the diff on save to clang-format's vim integration.

Ping? Do you think this is useful?

Apr 26 2017, 5:39 AM