Page MenuHomePhabricator

new clang-tidy checker readability-non-const-parameter
ClosedPublic

Authored by danielmarjamaki on Dec 8 2015, 6:16 AM.

Details

Summary

This is a new clang-tidy checker that will warn when function parameters should be const.

See also related discussions in http://reviews.llvm.org/D12359

As usual, I am testing this checker on the debian projects. I have so far triaged 20 warnings from this clang-tidy checker and they were all true positives.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This check partially implements PR19419. Could it be extended to variables?

This check partially implements PR19419. Could it be extended to variables?

Yes, I don't see any technical reasons why that would not work. However politically, some people like const variables and others don't.

This check partially implements PR19419. Could it be extended to variables?

Yes, I don't see any technical reasons why that would not work. However politically, some people like const variables and others don't.

May be implement as separate check with some code sharing if applicable?

danielmarjamaki edited edge metadata.

Run this check on C++ code also. I have rerun the add_new_check python script. Minor code cleanups.

I have run this on the debian packages again. In 1806 projects there were 9691 warnings. I have so far triaged 51 of those warnings and they were all true positives as far as I saw.

I see no warning when running on Clang source code (files in llvm/tools/clang/lib/...).

I see no warning when running on Clang source code (files in llvm/tools/clang/lib/...).

For information I rerun with --header-filter=* and got some results.. I will triage those..

LegalizeAdulthood added inline comments.
docs/clang-tidy/checks/readability-non-const-parameter.rst
15 ↗(On Diff #47962)

With pointers, there are always two layers of constness:

  • Whether the pointer itself is modified
  • Whether the data pointed to is modified

When you say "p should be const", I read that as the former.

I am pretty sure you intended the latter. You should be more explicit in your documentation. The ambiguity would have been resolved if you showed the "after" version of the code that would eliminate the warning. This is semi-implied by your next example, but it's kinder to the reader to resolve this ambiguity immediately.

32 ↗(On Diff #47962)

Here you are using "making p const" in the first sense I noted above.

It might help if you say "make *p const" when you mean that p points to constant data and use "making p const" when you mean that the pointer itself is unmodified.

There is also the wart in C++ that one can declare a function like this:

int f(char *p);

and define it like this:

int f(char *const p) {
  // ...
}

My sense is that this is pretty confusing to most programmers who really want the declaration and definition to be textually identical.

test/clang-tidy/readability-non-const-parameter.cpp
3 ↗(On Diff #47962)

How hard is it to extend it to references?

Certainly the confusion about what is const is easier to resolve in the case of references because the references themselves are immutable.

danielmarjamaki marked 2 inline comments as done.Feb 16 2016, 11:39 PM
danielmarjamaki added inline comments.
docs/clang-tidy/checks/readability-non-const-parameter.rst
15 ↗(On Diff #47962)

ok I understand, will try to improve

test/clang-tidy/readability-non-const-parameter.cpp
3 ↗(On Diff #47962)

If a "int &" reference parameter is not written then probably it's better to pass it by value than making it const. I would prefer that unless it has to use "int &" to comply with some interface.

I realize that the same can be said about pointers. If there is a "int *p" and you just read the value that p points at .. maybe sometimes it would be preferable to pass it by value.

test/clang-tidy/readability-non-const-parameter.cpp
3 ↗(On Diff #47962)

I thought the point of this check was to identify stuff passed by reference/pointer that could instead be passed by const reference/pointer.

Passing a read-only number type (short, char, int, float, double, etc.) parameter by pointer or by reference/pointer is a code smell, but this check isn't trying to identify that situation.

I'm more interested in things like:

void foo(std::string &s);

becoming

void foo(const std::string &s);

when s is treated as a read-only value within the implementation of foo.

danielmarjamaki added inline comments.Mar 8 2016, 2:20 AM
test/clang-tidy/readability-non-const-parameter.cpp
3 ↗(On Diff #47962)

I agree pointers and references are related from user perspective.

But technically I think we can't reuse the same markCanNotBeConst code.

This expression "p += 2" does not prevent that *p is const. But "r += 2" would prevent that the reference r is const.

I'm more interested in things like:

Yes that would be interesting. However I only warn about number types right now. As my last example in the rst file shows I don't warn for non-constant struct pointers for a reason.

Imho, I would like to have a warning when the data is modified even though *p is const. Like this:

struct S { int *a; int *b; };
int f3(const struct S * const p) {
  *(p->a) = 0; // <- I would like to have warning here
}

Also dealing with C++ is not my priority. I write this check so I can use it on C code.

Updated documentation

danielmarjamaki marked 7 inline comments as done.Mar 8 2016, 3:49 AM
danielmarjamaki added inline comments.
docs/clang-tidy/checks/readability-non-const-parameter.rst
16 ↗(On Diff #50032)

thanks. I have tried to make it more clear in latest patch.

33 ↗(On Diff #50032)

thanks. fixed in latest patch.

danielmarjamaki marked 2 inline comments as done.Mar 8 2016, 3:56 AM
danielmarjamaki added inline comments.
test/clang-tidy/readability-non-const-parameter.cpp
4 ↗(On Diff #50032)

Imho, I would like to have a warning when the data is modified even though *p is const. Like this:

hmm.. sorry about that.. that is of course out-of-scope for this check so not relevant to discuss here.

hokein added inline comments.Mar 14 2016, 7:11 AM
clang-tidy/readability/NonConstParameterCheck.cpp
24 ↗(On Diff #50032)

Extra . at the end.

124 ↗(On Diff #50032)

const auto &It

clang-tidy/readability/NonConstParameterCheck.h
53 ↗(On Diff #50032)

s/are/is

docs/clang-tidy/checks/readability-non-const-parameter.rst
6 ↗(On Diff #50032)

Looks like what the document says isn't consistent with the check, since the check only finds non-const pointer parameter.

test/clang-tidy/readability-non-const-parameter.cpp
3 ↗(On Diff #50032)

It makes sense to move this document to the rst, I think.

15 ↗(On Diff #50032)

Just keep the whole warning message in the first CHECK-MESSAGE.
And remove [readability-non-const-parameter] in others CHECK-MESSAGE for keeping the line short.

219 ↗(On Diff #50032)

Please add a test case:

class C {
public:
  C(int *p) : p(p) {}
private:
  const int *p;
};

BTW, does the check support class method?

danielmarjamaki marked 12 inline comments as done.Mar 15 2016, 2:47 AM
danielmarjamaki added inline comments.
docs/clang-tidy/checks/readability-non-const-parameter.rst
6 ↗(On Diff #50032)

I changed "Finds function parameters.." to "Finds function pointer parameters..".

test/clang-tidy/readability-non-const-parameter.cpp
3 ↗(On Diff #50032)

Done. First line in rst will now say: "Finds function pointer parameters that should be const".

219 ↗(On Diff #50032)

Thanks, I added such class.

I also added one class that shows that class methods are supported.

danielmarjamaki marked 3 inline comments as done.

Fixed review comments

rsmith added inline comments.Mar 18 2016, 8:32 AM
test/clang-tidy/readability-non-const-parameter.cpp
116–134 ↗(On Diff #50705)

The wording of this warning, and the name of the check, are highly misleading.

It's *not* the parameter that can be const, it's the parameter's *pointee*.

test/clang-tidy/readability-non-const-parameter.cpp
116–134 ↗(On Diff #50705)

I understand. Figuring out a better message is not easy. I and a collegue brain stormed:

pointer parameter 'p' could be declared with const 'const int *p'

pointer parameter 'p' could be pointer to const

pointer parameter 'p' declaration could be pointer to const

missing const in pointer parameter 'p' declaration

declaration of pointer parameter 'p' could be 'const int *p'

Do you think any of these would be good?

test/clang-tidy/readability-non-const-parameter.cpp
116–134 ↗(On Diff #50705)

Regarding the name of the check, I can't think of anything better only things that are longer but aren't substantially more clear in revealing the intent of the check. IMO, you don't blindly run clang-tidy checks based on the name, you run them after reading what they do and deciding if you want that transformation or not.

In order of my preference:

  • pointer parameter 'p' could be pointer to const
  • declaration of pointer parameter 'p' could be 'const int *p'

I might use the word 'can' instead of the world 'could', but that's a very minor quibble.

alexfh added inline comments.Apr 1 2016, 6:37 PM
clang-tidy/readability/NonConstParameterCheck.cpp
44 ↗(On Diff #50705)

const auto *Ctor

docs/clang-tidy/checks/readability-non-const-parameter.rst
7 ↗(On Diff #50705)

That now sounds ambiguous (is it about parameters of a pointer to a function type?). I'd say "The check finds function parameters of a pointer type that could be changed to point to a constant type instead." or something similar.

8 ↗(On Diff #50705)

"prevent unintentional modification of data"

10 ↗(On Diff #50705)

"make it easier for developers ..."

12 ↗(On Diff #50705)

s/clang-tidy/This check/

test/clang-tidy/readability-non-const-parameter.cpp
244 ↗(On Diff #50705)

Please add a test to ensure that overridden methods are not modified (since you would need to update the base class' method and then all the other overrides.

251 ↗(On Diff #50705)

The check can break code taking a pointer to a function it modifies:

typedef int (*fn)(int *);
int f(int *p) {return *p; }
fn fp = &f;

It's not always possible to detect, when this happens (for example, a pointer to the function could be taken in a different translation unit), but we should at least keep this case in mind. Please add a test case for this.

alexfh requested changes to this revision.Apr 1 2016, 6:37 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 1 2016, 6:37 PM

Please also mention check in docs/ReleaseNotes.rst.

docs/clang-tidy/checks/readability-non-const-parameter.rst
15 ↗(On Diff #50705)

I think you should add ".. code-block:: c++" before code block.

danielmarjamaki edited edge metadata.

Fix patch so it can be applied in latest trunk. Tried to fix review comments.

danielmarjamaki marked 10 inline comments as done.Aug 10 2016, 3:23 AM
danielmarjamaki added inline comments.
test/clang-tidy/readability-non-const-parameter.cpp
117–135 ↗(On Diff #67500)

I have changed the message now. pointer parameter 'p' can be pointer to const

The name is the same. Do you think it would be better with PointerParameterConstnessCheck() perhaps?

danielmarjamaki edited edge metadata.

fixed more review comments

danielmarjamaki marked 2 inline comments as done.Aug 10 2016, 4:49 AM
danielmarjamaki added inline comments.
test/clang-tidy/readability-non-const-parameter.cpp
245 ↗(On Diff #67504)

I have added tests and fixed FPs in latest diff.

252 ↗(On Diff #67504)

I have added a test in latest diff. but there is a false positive. I believe I heard about whole program analysis in clang-tidy sometime.. is that something worked on / implemented yet?

danielmarjamaki marked 2 inline comments as done.

added this check to the release notes. run clang-format.

Prazek added a subscriber: Prazek.Aug 12 2016, 9:47 AM
Prazek added inline comments.
clang-tidy/readability/NonConstParameterCheck.cpp
95–98 ↗(On Diff #67506)

This looks like it could be in the same if.

clang-tidy/readability/NonConstParameterCheck.cpp
95–98 ↗(On Diff #67506)

Yes it could. But would it make the code more or less readable? It wouldn't be a 1-line condition anymore then.

alexfh requested changes to this revision.Aug 16 2016, 7:58 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/readability/NonConstParameterCheck.cpp
95–98 ↗(On Diff #67506)

I also think that it makes sense to merge the conditions. The problem with the current code is that it is suspicious ("Why is the same action is done in two branches? Is it a bug?"). One line condition vs two lines seems secondary in this case.

103 ↗(On Diff #67506)

This seems too strict. What about other primitive types?

docs/clang-tidy/checks/readability-non-const-parameter.rst
22 ↗(On Diff #67506)

Please put braces on the same line with the function header. We should keep documentation consistent with LLVM style (where there's no good reason to do otherwise).

test/clang-tidy/readability-non-const-parameter.cpp
210 ↗(On Diff #67506)

Put braces on the previous line, please. A few other instances below.

238 ↗(On Diff #67506)

Add a CHECK-FIXES, please.

253 ↗(On Diff #67506)

Clang-tidy can't yet analyze multiple translation units simultaneously, so we just need to keep cases that can't be correctly handled by analyzing a single translation unit at a time in mind (and also in documentation and in tests), so that they are less surprising to the user.

However, the check can and should take care of correctly handling this case in a single translation unit (as in this test, use_functionpointer2). I'm not sure it's a frequent case, so I don't insist on fixing this right away. May be fine for a follow up.

This revision now requires changes to proceed.Aug 16 2016, 7:58 AM
danielmarjamaki marked 6 inline comments as done.Aug 18 2016, 3:53 AM
danielmarjamaki added inline comments.
clang-tidy/readability/NonConstParameterCheck.cpp
95–98 ↗(On Diff #67506)

ok

103 ↗(On Diff #67506)

I am not sure which type you are talking about. As far as I see we're writing warnings about bool,char,short,int,long,long long,float,double,long double,enum pointers.

I have intentionally avoided records now to start with. It should be added, but we need to be more careful when we do it.

test/clang-tidy/readability-non-const-parameter.cpp
210 ↗(On Diff #67506)

sorry .. of course I should run clang-format on this.

danielmarjamaki edited edge metadata.
danielmarjamaki marked 2 inline comments as done.

Fixed review comments

danielmarjamaki edited edge metadata.

Fixed review comments about formatting in doc

alexfh accepted this revision.Aug 18 2016, 8:34 AM
alexfh edited edge metadata.

LG

clang-tidy/readability/NonConstParameterCheck.cpp
104 ↗(On Diff #68531)

You're right. I was thinking about enums, but isIntegerType takes care of them too. As the next step, we should try to add all POD types, but that's a question for another patch.

test/clang-tidy/readability-non-const-parameter.cpp
118–136 ↗(On Diff #68531)

PointerParameterConstnessCheck is not much better, IMO. Maybe readability-use-pointer-to-const-parameter? Or just leave it like this for now.

This revision is now accepted and ready to land.Aug 18 2016, 8:34 AM
This revision was automatically updated to reflect the committed changes.