Page MenuHomePhabricator

[clang-tidy] Add check for function parameters that are const& to builtin types
Needs RevisionPublic

Authored by sdowney on Mar 15 2016, 9:53 AM.

Details

Reviewers
alexfh
sbenza
Summary

Add a check for const& to builtin types. Add FixIt replacing, for example, 'const& int i' with 'int i'.

Diff Detail

Event Timeline

sdowney updated this revision to Diff 50749.Mar 15 2016, 9:53 AM
sdowney retitled this revision from to [clang-tidy] Add check for function parameters that are const& to builtin types.
sdowney updated this object.
sdowney added a reviewer: alexfh.
sdowney added a subscriber: cfe-commits.

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

test/clang-tidy/misc-const-ref-builtin.cpp
33–35

I see tests here only for declarations, and not definitions of functions.

I also don't see any tests for:

  • member functions
  • template member functions
  • template specializations
  • tests where function signatures result from macro expansion, either in the body of a macro or as a macro argument

drive-by, some comments.

clang-tidy/misc/ConstRefBuiltinCheck.cpp
52

Function is deferenced, but it's assumed possibly null at line 34 -> dyn_cast_or_null
If not, then you should use dyn_cast instead.

docs/clang-tidy/checks/misc-const-ref-builtin.rst
29

nit: instantion -> instantiation

sdowney updated this revision to Diff 50982.Mar 17 2016, 3:34 PM

Add more test cases, covering templates, template specializations, function definitions, member function templates.

Check if parent function of the argument is a nullptr.

Fix spelling mistake.

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

The usual pattern for that is to have the definition use const, and the declaration not, since it's an implementation detail if the passed parameter is modified. And, unfortunately, I'm stuck with one compiler that mangles those two differently, causing link errors.

Having void foo(const int i) in a declaration makes me suspicious, since the const is meaningless.

I could make this an option? If the option is set, emit 'const type' if the Decl hasBody?

sdowney updated this revision to Diff 50986.Mar 17 2016, 4:20 PM

Add tests with functions generated with MACROS

sdowney updated this revision to Diff 51000.Mar 17 2016, 5:21 PM

Add tests for FixIts

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

The usual pattern for that is to have the definition use const, and the declaration not, since it's an implementation detail if the passed parameter is modified. And, unfortunately, I'm stuck with one compiler that mangles those two differently, causing link errors.

Having void foo(const int i) in a declaration makes me suspicious, since the const is meaningless.

I could make this an option? If the option is set, emit 'const type' if the Decl hasBody?

It seems reasonable to do this as a refinement after this check is in place.

alexfh edited edge metadata.Apr 1 2016, 7:29 PM

Adding Samuel, who's written a similar check internally and might want to upstream it or suggest improvements to this patch.

sbenza edited edge metadata.Apr 4 2016, 7:44 AM

As Alex mentioned, we have a test like this.
It also adds a hardcoded list of user-defined types that are known to be better when passed by value (eg. StringRef)

One big difference is that we decided to not trigger on typedefs.
We can't know that the typedef is documented to be trivial and it could change in the future.
The check actually verifies that the spelling is the expected spelling.
That skips things like macros, templates, type traits, typedefs, aliases, etc.

I could upstream that check and make the user-defined type list configurable.

At least in my codebase, skipping templates is too strong. I run across ones where the const& parameter is not one controlled by a template. It's often a size_t.

I could easily see not fixing the typedef'd refs, also, although I think warning on them is still useful. Particularly if they can then be added to a list to be changed. E.g. size_t.

sbenza added a comment.Apr 4 2016, 8:51 AM

At least in my codebase, skipping templates is too strong. I run across ones where the const& parameter is not one controlled by a template. It's often a size_t.

The only check we are doing (other than matching type) is matching spelling.
This means that it will only skip templates where that type is spelled, for example, using the template argument.

So it will skip this template <typename T> void Foo(const T&); but not this template <typename T> void Foo(const T&, const size_t&); (for the size_t argument).
It will also skip things like void Foo(const T&, const typename T::difference_type&);

The idea is that if the spelling is not exact, then there is some external factor that can change what the type means and make the by-value option less efficient.

I could easily see not fixing the typedef'd refs, also, although I think warning on them is still useful. Particularly if they can then be added to a list to be changed. E.g. size_t.

Warning on types that will never be added to the list and that should be taken by const& will lead to constant false positives.
We could make these warnings opt-in. This way some users might turn it on always or just turn it on to find the list of types.

alexfh added inline comments.Apr 7 2016, 10:58 AM
docs/clang-tidy/checks/misc-const-ref-builtin.rst
28

nit: enclose g with double backquotes, please. Same with other inline code snippets (int, double, etc.)

alexfh requested changes to this revision.Apr 15 2016, 4:54 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 15 2016, 4:54 AM