[PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Aaron Ballman via cfe-commits]

On Wed, Sep 2, 2015 at 2:19 AM, Daniel Marjamäki
<daniel.marjamaki at evidente.se> wrote:
> danielmarjamaki added a comment.
>
>> > > The checker isn't currently path sensitive as it doesn't pay attention
>
>>
>
>> >
>
>>
>
>> > >  to control flow graphs or how pointer values flow through a function
>
>>
>
>> >
>
>>
>
>> > >  body. I suppose this is a matter of scope more than anything; I see a
>
>>
>
>> >
>
>>
>
>> > >  logical extension of this functionality being with local variables as
>
>>
>
>> >
>
>>
>
>> > >  well as parameters. So, for instance:
>
>>
>
>> >
>
>>
>
>> > >
>
>>
>
>> >
>
>>
>
>> > > void f(int *p) {
>
>>
>
>> >
>
>>
>
>> > >
>
>>
>
>> >
>
>>
>
>> > >   int *i = p;
>
>>
>
>> >
>
>>
>
>> > >   std::cout << *i;
>
>>
>
>> >
>
>>
>
>> > >
>
>>
>
>> >
>
>>
>
>> > > }
>
>>
>
>> >
>
>>
>
>> >
>
>>
>
>> > Imho that path analysis is not very interesting. The "p" can't be const until we see that "i" is const.
>
>>
>
>>
>
>> Correct, but from the user's perspective, why are we not telling them
>
>>  both can be const?
>
>
> We want to have simple and clear warning messages. I will currently just write "parameter p can be const." Imho that is simple and clear. In your example I believe it would be required with a more complex message. Because p can't be const. It can only be const if i is made const first.

That is true.

> As I see it "my" analysis does not have any false negatives that would be avoided. It's just that 2 separate simple messages are clumped together into 1 more complex message.

Fair.

> I also believe that solving this iteratively in small steps is less error prone.

Definitely agreed.

>
>> > if we talk about the user interface.. imho it would be nice that this is a compiler warning. the analysis is quick and there should be little noise.
>> I'm not certain about the performance of the analysis (I suspect it's
>>  relatively cheap though), but we do not usually want off-by-default
>>  warnings in the frontend, and I suspect that this analysis would have
>>  to be off by default due to the chattiness on well-formed code.
>
> hmm.. I believe that this is common practice. I can see that people want to turn it off for legacy code though.

Legacy code is what I'm most concerned about. These diagnostics aren't
warning the user that their code is incorrect in some way, they're
it's showing where the code could be safer with improved
const-correctness. If it's producing 30 new diagnostics per project on
average, that's a lot of noise for very little benefit to those
projects. We usually put those sort of diagnostics in other parts of
the compiler (analysis, clang-tidy, etc). Experience has shown that
off-by-default warnings generally never get turned on.

> but we can look at the warnings on real code and discuss those. I have results from Clang.
>
>
> ================
> Comment at: lib/Sema/SemaDecl.cpp:10334
> @@ +10333,3 @@
> +      continue;
> +    // To start with we don't warn about structs.
> +    if (T->getPointeeType().getTypePtr()->isRecordType())
> ----------------
> aaron.ballman wrote:
>> This seems *really* limiting (especially for C++ code), why the restriction?
> 2 reasons...
>
> 1. I don't think we should warn for structs whenever it's possible to make them const.
>
> Example code:
>
>     struct FRED { char *str; };
>     void f(struct FRED *fred) {
>         strcpy(fred->str, "abc");
>     }
>
> fred can be const but imho we should not warn about this. Imho it would be misleading to make fred const.

Agreed. However, as a counter-point:

struct S {
  void f() const;
};

void f(S *s) {
  s->f();
}

> 2. I wanted to keep the scope of this checker limited to start with. If we want to warn about structs also I need to write much more code in the MarkWritten etc.

That's reasonable, but it would be good to put some test cases in as a
FIXME for code that you think should be handled eventually, but isn't
handled currently.

~Aaron

>
>
>
> http://reviews.llvm.org/D12359
>
>
>