[PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Aaron Ballman via cfe-commits]

On Wed, Feb 17, 2016 at 2:39 AM, Daniel Marjamäki
<daniel.marjamaki at evidente.se> wrote:
> danielmarjamaki marked 2 inline comments as done.
>
> ================
> Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:15
> @@ +14,3 @@
> +
> +  // warning here; p should be const
> +  char f1(char *p) {
> ----------------
> LegalizeAdulthood wrote:
>> 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.
>>
>>
> ok I understand, will try to improve
>
> ================
> Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
> @@ +2,3 @@
> +
> +// Currently the checker only warns about pointer arguments.
> +//
> ----------------
> LegalizeAdulthood wrote:
>> 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.
> 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 think it makes sense to pass it by value only if it is not an
expensive-to-copy type (we have an AST matcher helper for that in
TypeTraits.h), or to comply with an interface.

~Aaron

>
> 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.
>
>
> http://reviews.llvm.org/D15332
>
>
>