Page MenuHomePhabricator

flx (Felix Berger)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 14 2015, 7:53 AM (214 w, 1 d)

Recent Activity

Sep 26 2018

flx added a comment to D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy.

Would that also skip checks for something like shared_ptr?

Yes, everything ending on pointer, ptr, reference or ref, first letter case insensitive.

Sep 26 2018, 7:10 AM · Restricted Project

Sep 20 2018

flx added inline comments to D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy.
Sep 20 2018, 10:41 AM · Restricted Project

Jul 25 2017

flx committed rL309067: [clang-tidy] Do not issue fixit for explicit template specializations.
[clang-tidy] Do not issue fixit for explicit template specializations
Jul 25 2017, 5:46 PM
flx closed D35718: [clang-tidy] Do not issue fixit for explicit template specializations by committing rL309067: [clang-tidy] Do not issue fixit for explicit template specializations.
Jul 25 2017, 5:46 PM

Jul 22 2017

flx updated the diff for D35718: [clang-tidy] Do not issue fixit for explicit template specializations.
Jul 22 2017, 7:08 AM

Jul 20 2017

flx created D35718: [clang-tidy] Do not issue fixit for explicit template specializations.
Jul 20 2017, 9:36 PM

Jan 19 2017

flx committed rL292491: [clang-tidy] Do not trigger move fix for non-copy assignment operators in….
[clang-tidy] Do not trigger move fix for non-copy assignment operators in…
Jan 19 2017, 8:02 AM
flx closed D28899: [clang-tidy] Do not trigger move fix for non-copy assignment operators in performance-unnecessary-value-param check by committing rL292491: [clang-tidy] Do not trigger move fix for non-copy assignment operators in….
Jan 19 2017, 8:02 AM
flx created D28899: [clang-tidy] Do not trigger move fix for non-copy assignment operators in performance-unnecessary-value-param check.
Jan 19 2017, 6:06 AM

Dec 15 2016

flx committed rL289912: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
[clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop
Dec 15 2016, 6:58 PM
flx closed D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop by committing rL289912: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
Dec 15 2016, 6:58 PM · Restricted Project
flx updated the diff for D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
Dec 15 2016, 5:12 PM · Restricted Project

Dec 2 2016

flx committed rL288502: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as….
[clang-tidy] Do not trigger unnecessary-value-param check on methods marked as…
Dec 2 2016, 6:54 AM
flx closed D27248: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final by committing rL288502: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as….
Dec 2 2016, 6:54 AM · Restricted Project
flx added inline comments to D27248: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final.
Dec 2 2016, 6:42 AM · Restricted Project

Dec 1 2016

flx added a comment to D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.

Do you have any more comments, Alex?

Dec 1 2016, 7:51 PM · Restricted Project

Nov 30 2016

flx added inline comments to D27248: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final.
Nov 30 2016, 6:44 AM · Restricted Project
flx updated the diff for D27248: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final.
Nov 30 2016, 6:44 AM · Restricted Project

Nov 29 2016

flx retitled D27248: [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final from to [clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final.
Nov 29 2016, 8:34 PM · Restricted Project
flx added inline comments to D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
Nov 29 2016, 7:46 AM · Restricted Project
flx added inline comments to D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
Nov 29 2016, 5:51 AM · Restricted Project
flx updated the diff for D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
Nov 29 2016, 5:48 AM · Restricted Project

Nov 28 2016

flx retitled D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop from to [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop.
Nov 28 2016, 6:45 PM · Restricted Project

Nov 11 2016

flx added a comment to D26453: [clang-tidy] Remove duplicated check from move-constructor-init.

Add ValuesOnly option to modernize-pass-by-value.

Nov 11 2016, 9:14 AM

Nov 9 2016

flx committed rL286424: [clang-tidy] Do not issue fix for functions that are referenced outside of….
[clang-tidy] Do not issue fix for functions that are referenced outside of…
Nov 9 2016, 5:38 PM
flx closed D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr by committing rL286424: [clang-tidy] Do not issue fix for functions that are referenced outside of….
Nov 9 2016, 5:38 PM
flx added a comment to D26453: [clang-tidy] Remove duplicated check from move-constructor-init.

Is the modernize-pass-by-value check configurable in a way to only trigger when copied constructor arguments are not moved?

Nov 9 2016, 8:42 AM
flx added a comment to D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr.

Thanks for the review!

Nov 9 2016, 6:42 AM
flx updated the diff for D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr.
Nov 9 2016, 6:42 AM

Nov 7 2016

flx committed rL286155: [clang-tidy] Move incomplete type test into separate test file.
[clang-tidy] Move incomplete type test into separate test file
Nov 7 2016, 1:55 PM
flx closed D26369: [ClangTidy] Move incomplete type test into separate test file by committing rL286155: [clang-tidy] Move incomplete type test into separate test file.
Nov 7 2016, 1:55 PM
flx added inline comments to D26369: [ClangTidy] Move incomplete type test into separate test file.
Nov 7 2016, 1:52 PM
flx added inline comments to D26369: [ClangTidy] Move incomplete type test into separate test file.
Nov 7 2016, 1:47 PM
flx updated the diff for D26369: [ClangTidy] Move incomplete type test into separate test file.
Nov 7 2016, 1:46 PM
flx added a reviewer for D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr: aaron.ballman.
Nov 7 2016, 1:42 PM
flx added a comment to D26195: Ignore incomplete types when determining whether they are expensive to copy.
Nov 7 2016, 1:20 PM
flx retitled D26369: [ClangTidy] Move incomplete type test into separate test file from to [ClangTidy] Move incomplete type test into separate test file.
Nov 7 2016, 1:19 PM

Nov 4 2016

flx committed rL286010: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current….
[ClangTidy - performance-unnecessary-value-param] Only add "const" when current…
Nov 4 2016, 2:01 PM
flx closed D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified by committing rL286010: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current….
Nov 4 2016, 2:01 PM
flx committed rL286008: [clang-tidy] Ignore incomplete types when determining whether they are….
[clang-tidy] Ignore incomplete types when determining whether they are…
Nov 4 2016, 1:39 PM
flx closed D26195: Ignore incomplete types when determining whether they are expensive to copy by committing rL286008: [clang-tidy] Ignore incomplete types when determining whether they are….
Nov 4 2016, 1:38 PM
flx updated the diff for D26195: Ignore incomplete types when determining whether they are expensive to copy.
Nov 4 2016, 1:23 PM
flx added a comment to D26195: Ignore incomplete types when determining whether they are expensive to copy.
In D26195#585198, @flx wrote:
In D26195#584958, @flx wrote:
In D26195#584724, @flx wrote:

Please add a test case with an incomplete type that would exercise this code path, otherwise, LGTM.

Hi Aaron,

do you have any advise on how to add an incomplete type? When debugging this I had a compilation unit that failed to compile causing it, but I'm not sure this is a good way to add a test case.

A type like class C; is an incomplete type, as is void, so perhaps you can find a check that would let such a construct call through to isExpensiveToCopy().

Great, this works and I was able to see the check produce a false positive without the proposed change here, but the test code introduces a compile error now due to the incomplete type used in the function definition. Is there a way to suppress that?

Unlikely -- fixing the compile error likely makes the type not expensive to copy by using a pointer (or reference). This may be tricky to test because the times when you would call isExpensiveToCopy() is with types that are going to be logically required to be complete. I am not certain the compile error is actually a problem though -- I would imagine your existing false-positives (that you mentioned in the patch summary) are cases where there is a compile error *and* a clang-tidy diagnostic, so the test may simply be "check that there's only a compile error and no clang-tidy diagnostic where there used to be a false-positive one."

That's exactly the case, my question here is how can I make the test succeed in the face of a compile error, i.e. by expecting the error as well?

Doesn't // CHECK-MESSAGES: :[[@LINE-1]]:3: error: blah blah blah work?

Nov 4 2016, 12:12 PM
flx added a reviewer for D26195: Ignore incomplete types when determining whether they are expensive to copy: aaron.ballman.
Nov 4 2016, 12:11 PM
flx updated the diff for D26195: Ignore incomplete types when determining whether they are expensive to copy.
Nov 4 2016, 12:10 PM
flx added a comment to D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.

Aaron, do you have any other comments or does the patch look good to you?

Nov 4 2016, 11:07 AM

Nov 3 2016

flx added inline comments to D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
Nov 3 2016, 12:11 PM
flx updated the diff for D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
Nov 3 2016, 12:11 PM

Nov 2 2016

flx added inline comments to D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
Nov 2 2016, 2:59 PM

Nov 1 2016

flx added a comment to D26195: Ignore incomplete types when determining whether they are expensive to copy.
In D26195#584958, @flx wrote:
In D26195#584724, @flx wrote:

Please add a test case with an incomplete type that would exercise this code path, otherwise, LGTM.

Hi Aaron,

do you have any advise on how to add an incomplete type? When debugging this I had a compilation unit that failed to compile causing it, but I'm not sure this is a good way to add a test case.

A type like class C; is an incomplete type, as is void, so perhaps you can find a check that would let such a construct call through to isExpensiveToCopy().

Great, this works and I was able to see the check produce a false positive without the proposed change here, but the test code introduces a compile error now due to the incomplete type used in the function definition. Is there a way to suppress that?

Unlikely -- fixing the compile error likely makes the type not expensive to copy by using a pointer (or reference). This may be tricky to test because the times when you would call isExpensiveToCopy() is with types that are going to be logically required to be complete. I am not certain the compile error is actually a problem though -- I would imagine your existing false-positives (that you mentioned in the patch summary) are cases where there is a compile error *and* a clang-tidy diagnostic, so the test may simply be "check that there's only a compile error and no clang-tidy diagnostic where there used to be a false-positive one."

Nov 1 2016, 2:21 PM
flx added inline comments to D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
Nov 1 2016, 2:19 PM
flx updated the diff for D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
Nov 1 2016, 2:18 PM
flx retitled D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified from to [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
Nov 1 2016, 11:20 AM
flx added a comment to D26195: Ignore incomplete types when determining whether they are expensive to copy.
In D26195#584724, @flx wrote:

Please add a test case with an incomplete type that would exercise this code path, otherwise, LGTM.

Hi Aaron,

do you have any advise on how to add an incomplete type? When debugging this I had a compilation unit that failed to compile causing it, but I'm not sure this is a good way to add a test case.

A type like class C; is an incomplete type, as is void, so perhaps you can find a check that would let such a construct call through to isExpensiveToCopy().

Nov 1 2016, 11:09 AM
flx retitled D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr from to [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr.
Nov 1 2016, 10:00 AM
flx added a comment to D26195: Ignore incomplete types when determining whether they are expensive to copy.

Please add a test case with an incomplete type that would exercise this code path, otherwise, LGTM.

Nov 1 2016, 9:32 AM
flx retitled D26195: Ignore incomplete types when determining whether they are expensive to copy from to Ignore incomplete types when determining whether they are expensive to copy.
Nov 1 2016, 8:44 AM

Jul 5 2016

flx committed rL274552: [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods.
[clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods
Jul 5 2016, 7:48 AM
flx closed D21936: [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods by committing rL274552: [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods.
Jul 5 2016, 7:48 AM
flx added inline comments to D21936: [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods.
Jul 5 2016, 7:42 AM
flx updated the diff for D21936: [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods.
Jul 5 2016, 7:42 AM

Jul 1 2016

flx retitled D21936: [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods from to [clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods.
Jul 1 2016, 2:01 PM
flx committed rL274380: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const….
[clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const…
Jul 1 2016, 1:19 PM
flx closed D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved. by committing rL274380: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const….
Jul 1 2016, 1:19 PM
flx added inline comments to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
Jul 1 2016, 12:40 PM
flx updated the diff for D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..

Thanks for the review. I addressed all remaining comments.

Jul 1 2016, 12:40 PM

May 31 2016

flx updated the diff for D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 31 2016, 8:08 PM
flx added inline comments to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 31 2016, 5:11 PM
flx updated the diff for D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 31 2016, 5:11 PM
flx added a comment to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
In D20277#436725, @flx wrote:

Cool check! Did you think about sugesting std::move for rvalue references if they are used once?

Thanks! I'm not sure this fits with what a user would expect from a check named "unnecessary-value-param" since in this case the parameter is not passed by value. Would a separate check make sense for this?

Consider changing the name :) When I was thinking about the check for rvalue references, I wanted to name it "*-rvalue-ref-one-use".
If it covers values also, maybe "performance-move-single-used-variable" or "performance-variable-one-use", or "performance-disposable-object" (If I translated it correctly).

In my opinion there is no need to have separate check to to this, because the user would like to have both or none.

May 31 2016, 7:58 AM

May 30 2016

flx added a comment to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..

Alex, Sam, could you take another look? It'd be great to get this change in to make the check more useful.

May 30 2016, 5:39 PM
flx committed rL271239: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const….
[clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const…
May 30 2016, 5:32 PM
flx closed D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified by committing rL271239: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const….
May 30 2016, 5:32 PM

May 26 2016

flx added a comment to D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified .

Friendly ping.

May 26 2016, 7:38 AM
flx added a comment to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..

This is ready for review again.

May 26 2016, 7:37 AM

May 23 2016

flx added a comment to D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified .

How many more (in relative numbers) results does this check generate now?

May 23 2016, 6:50 PM
flx updated the diff for D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified .
May 23 2016, 6:49 PM
flx added a comment to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..

Cool check! Did you think about sugesting std::move for rvalue references if they are used once?

May 23 2016, 8:05 AM
flx updated the diff for D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..

Upated documentation as well.

May 23 2016, 7:21 AM
flx added inline comments to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 23 2016, 6:59 AM
flx updated the diff for D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 23 2016, 6:58 AM

May 18 2016

flx added inline comments to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 18 2016, 7:31 AM
flx added a comment to D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..

Thanks for the feedback!

May 18 2016, 7:30 AM
flx updated the diff for D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 18 2016, 7:29 AM

May 15 2016

flx retitled D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved. from to [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
May 15 2016, 3:45 PM

May 14 2016

flx committed rL269581: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted….
[clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted…
May 14 2016, 3:50 PM
flx closed D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor. by committing rL269581: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted….
May 14 2016, 3:50 PM
flx added inline comments to D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 14 2016, 3:39 PM
flx updated the diff for D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 14 2016, 3:38 PM

May 12 2016

flx committed rL269389: [clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for….
[clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for…
May 12 2016, 7:54 PM
flx closed D19865: [clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for decl stmts with single VarDecl. by committing rL269389: [clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for….
May 12 2016, 7:54 PM
flx added inline comments to D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 12 2016, 7:44 PM
flx updated the diff for D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 12 2016, 7:44 PM
flx added inline comments to D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 12 2016, 7:23 AM
flx updated the diff for D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 12 2016, 7:23 AM

May 11 2016

flx retitled D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor. from to [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor..
May 11 2016, 7:49 AM

May 9 2016

flx added a comment to D19865: [clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for decl stmts with single VarDecl..
In D19865#423140, @flx wrote:
In D19865#419905, @flx wrote:

Is it a workaround to avoid breaking the code by incorrect fixes?

Yes. We can't simply change the type of DeclStmt when we only look one of the VarDecls and how it is initialized.

Also, all tests still pass. Alex, do you have any particular concern with this approach?

May 9 2016, 7:23 AM
flx updated the diff for D19865: [clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for decl stmts with single VarDecl..
May 9 2016, 7:22 AM

May 5 2016

flx added reviewers for D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified : alexfh, fowles.
May 5 2016, 7:22 PM
flx retitled D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified from to [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified .
May 5 2016, 7:22 PM