Page MenuHomePhabricator

Add performance check to flag function parameters of expensive to copy types that can be safely converted to const references.
ClosedPublic

Authored by flx on Feb 21 2016, 12:26 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 48636.Feb 21 2016, 12:26 PM
flx retitled this revision from to Add performance check to flag function parameters of expensive to copy types that can be safely converted to const references..
flx updated this object.
flx added a reviewer: alexfh.
flx set the repository for this revision to rL LLVM.
flx added a subscriber: cfe-commits.
alexfh added inline comments.Feb 22 2016, 7:13 AM
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
41 ↗(On Diff #48636)

Please add a comment about when this happens (template parameter packs? C-style variadic functions?).

46 ↗(On Diff #48636)

Please add an empty line before the comment.

48 ↗(On Diff #48636)

nit: Add three spaces before the first word for better alignment.

58 ↗(On Diff #48636)

s/making this a reference/making it a reference/?

62 ↗(On Diff #48636)

What if parameter doesn't have a name? Should we print an index ("parameter #n is copied ...").

docs/clang-tidy/checks/performance-unnecessary-value-param.rst
6 ↗(On Diff #48636)

Add an example?

16 ↗(On Diff #48636)

nit: a ; seems to be more suitable as a trailing punctuation here.

alexfh requested changes to this revision.Feb 24 2016, 5:44 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Feb 24 2016, 5:44 AM
flx updated this revision to Diff 49307.Feb 27 2016, 8:51 PM
flx edited edge metadata.
flx removed rL LLVM as the repository for this revision.
flx marked 6 inline comments as done.
flx added inline comments.
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
41 ↗(On Diff #48636)

I"m not sure when this can happen. Should I remove it and we add it if we find a case?

62 ↗(On Diff #48636)

Good point. Added test and index code.

alexfh added inline comments.Mar 1 2016, 7:48 AM
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
25–32 ↗(On Diff #49307)

How about return (Name.empty() ? llvm::Twine('#') + (Index + 1) : llvm::Twine('\'') + Name + '\'').str(); or something like this?

42 ↗(On Diff #49307)

Can you first try adding tests with template parameter packs and C-style variadic functions?

56 ↗(On Diff #49307)

nit: No braces around single-line if bodies.

fowles added a subscriber: fowles.Mar 22 2016, 8:43 AM
fowles added inline comments.
test/clang-tidy/performance-unnecessary-value-param.cpp
8 ↗(On Diff #49307)

you don't actually need to fill in these methods, just declare them

52 ↗(On Diff #49307)

no fix for this case?

flx updated this revision to Diff 51385.Mar 22 2016, 9:01 PM
flx updated this object.
flx edited edge metadata.
flx marked 5 inline comments as done.

After testing the check against a large corpus I was able to remove the unnecessary param index check. But I discovered that the check crashed on value arguments of deleted assignment operators due to the lack of a function body. I added a test case and added a guard to address this.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp
42 ↗(On Diff #49307)

Ran over large corpus and the check is not needed. Removed.

56 ↗(On Diff #49307)

Code is gone. Was not needed.

test/clang-tidy/performance-unnecessary-value-param.cpp
52 ↗(On Diff #49307)

Good point. Added.

alexfh accepted this revision.Mar 24 2016, 8:04 AM
alexfh edited edge metadata.

Looks good! Thank you!

clang-tidy/performance/UnnecessaryValueParamCheck.cpp
26 ↗(On Diff #51385)

nit: IIUC, there's no need for explicit llvm::Twine around the second '\'' in this line.

This revision is now accepted and ready to land.Mar 24 2016, 8:04 AM
flx added inline comments.Mar 26 2016, 2:55 PM
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
26 ↗(On Diff #51385)

Twine only has an implicit constructor for const char* but not for char. I can either leave it as is or switch to "'".

This revision was automatically updated to reflect the committed changes.

Can you add something to the docs/ReleaseNotes.rst for this new check, please?