This is an archive of the discontinued LLVM Phabricator instance.

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

Event Timeline

danielmarjamaki retitled this revision from to new clang-tidy checker readability-non-const-parameter.
danielmarjamaki updated this object.
danielmarjamaki added subscribers: cfe-commits, sberg.
aaron.ballman added inline comments.Dec 8 2015, 7:35 AM
clang-tidy/readability/NonConstParameterCheck.cpp
44

I think it may be an improvement to unify all of the matchers that will eventually be used to call markCanNotBeConst() from check() into a single matcher. e.g.,

Finder->addMatcher(
  anyOf(stmt(anyOf(binaryOperator(...), callExpr(), returnStmt(), unaryOperator(...))).bind("mark"),
             varDecl(...).bind("mark")), this);

The implementation of check() won't have to change too much, but this reduces the number of individual matchers required which I think may be a performance win (gut feeling, not based on evidence) and does not reduce clarity.

49

What will this do with code like:

int f(int *p) {
  void g();
  return *p;
}

or

int f(int *p) {
  struct S {
    void g();
  };
  return *p;
}
64

Better how?

90

This loop can be replaced with std::find_if().

125

These should all be const auto * because E is a const Expr *.

clang-tidy/readability/NonConstParameterCheck.h
20

I think this struct can be local to NonConstParameterCheck.

44

No need for the struct keyword, here and elsewhere.

45

addParm instead of addPar, for clarity?

46

I haven't gotten to the function definition yet, but I have no idea what this function would do. ;-) A more clear identifier would be appreciated.

alexfh added inline comments.Dec 8 2015, 8:51 AM
clang-tidy/readability/NonConstParameterCheck.cpp
21

I'd prefer the problem to be fixed right away instead of disabling this check for C++. The specific issue of parameters of template functions can probably be addressed by disabling matches in template instantiations (add unless(isInTemplateInstantiation()) or unless(isInstantiated()) depending on what kind of node is being matched).

84

Use the if (T *x = ...) {...} pattern, please.

90

While I agree with similar comments in general, it seems that here it wouldn't make the code any better.

169

if (ParInfo *PI = ...) ...

docs/clang-tidy/checks/readability-non-const-parameter.rst
15

Let's format the code examples in LLVM style.

test/clang-tidy/readability-non-const-parameter.c
211 ↗(On Diff #42168)

clang-format?

I have tried to fix the comments.

This patch is mostly untested. I have only run 'make check-all'. I am running more tests right now.

danielmarjamaki marked 12 inline comments as done.Dec 9 2015, 6:44 AM
danielmarjamaki added inline comments.
clang-tidy/readability/NonConstParameterCheck.cpp
45

I tried to unify like that. It almost worked. I failed to put the stmt(...) and varDecl(...) in the same matcher.

50

good question!

I tested and the checker seemed to handle these. apparently the ast-matcher scanned the functions recursively. However I don't know if we know that it will work, so I changed the checker.

85

I believe I fixed all these.

alexfh edited edge metadata.Dec 9 2015, 7:43 AM

Please add tests with templates and macros. Please also run this on LLVM/Clang to ensure a) it doesn't crash; b) the number of warnings is sane; c) a representative sample of warnings doesn't contain false positives.

danielmarjamaki marked 3 inline comments as done.Dec 9 2015, 8:00 AM

Please add tests with templates and macros. Please also run this on LLVM/Clang to ensure a) it doesn't crash; b) the number of warnings is sane; c) a representative sample of warnings doesn't contain false positives.

I am running clang-tidy on LLVM/Clang right now. I expect that it will take several hours.

I expect that I will get false positives when there are templates and I will add corresponding test cases.

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

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

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

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

ok I understand, will try to improve

test/clang-tidy/readability-non-const-parameter.cpp
3

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

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

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
17

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

34

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
5

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
25

Extra . at the end.

125

const auto &It

clang-tidy/readability/NonConstParameterCheck.h
54

s/are/is

docs/clang-tidy/checks/readability-non-const-parameter.rst
7

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
4

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

16

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.

220

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
7

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

test/clang-tidy/readability-non-const-parameter.cpp
4

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

220

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
117–135

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
117–135

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
117–135

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
45

const auto *Ctor

docs/clang-tidy/checks/readability-non-const-parameter.rst
8

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.

9

"prevent unintentional modification of data"

11

"make it easier for developers ..."

13

s/clang-tidy/This check/

test/clang-tidy/readability-non-const-parameter.cpp
245

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.

252

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
16

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
118–136

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
246

I have added tests and fixed FPs in latest diff.

253

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
96–99

This looks like it could be in the same if.

clang-tidy/readability/NonConstParameterCheck.cpp
96–99

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
96–99

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.

104

This seems too strict. What about other primitive types?

docs/clang-tidy/checks/readability-non-const-parameter.rst
23

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
211

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

239

Add a CHECK-FIXES, please.

254

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
96–99

ok

104

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
211

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
105

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
119–137

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.