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

[Aaron Ballman via cfe-commits]

On Mon, Aug 31, 2015 at 2:58 PM, Daniel Marjamäki
<daniel.marjamaki at evidente.se> wrote:
> danielmarjamaki added a comment.
>
> In http://reviews.llvm.org/D12359#236334, @aaron.ballman wrote:
>
>> I have concerns about this being a frontend warning. The true positive rate seems rather high given the benign nature of the diagnostic, and the false negative rate is quite high. This seems like it would make more sense as a path-sensitive static analyzer warning instead of a frontend warning, as that would justify the slightly high true positive rate, and rectify quite a bit of the false negative rate.
>
>
> I don't understand. The checker is path sensitive, isn't it? Do you see some problem that I don't?

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;
}

I think code like the above should tell the user that both p and i can be const.

> It will warn if there is no write anywhere in the function. Except as I wrote, for some cases where #ifdef is used, but moving it to static analysis won't help.

This is true, for the current design of the patch, static analysis is
less useful because the path sensitivity doesn't matter. But that also
suggests if we wanted to add such a thing to the static analyzer
someday, we'd have two ways of getting the same information if we
stuck this in the frontend. It seems more logical to me to set this up
as a static analysis checker so that we can extend it to be path
sensitive under the same flag.

>
>> Have you tried running this over the Clang and LLVM code bases? How many diagnostics does it produce?
>
>
> Not yet. I'll do that.
>
>
> ================
> Comment at: test/Sema/warn-nonconst-parameter.c:8
> @@ +7,3 @@
> +//
> +// It does not warn about pointers to records or function pointers.
> +
> ----------------
> aaron.ballman wrote:
>> How does it handle cases like:
>>
>> void g(int);
>> void f(volatile int *p) {
>>   int j = *p; // Should not warn
>>   int i = p[0]; // Should not warn
>>   g(*p); // Should not warn
>> }
>>
>> void h(int *p) {
>>   int i = p ? *p : 0; // Should warn
>> }
>>
> ok interesting. I have never seen a volatile pointer argument before. but technically I believe we should warn about f(). the function only reads p. maybe for stylistic reasons it would look weird to say that it's both volatile and const, is that why we should not warn?

I was thinking that we should not warn in those cases because volatile
values can be changed in ways unknown to the implementation. But now
that I'm a bit more awake, I'm not as convinced of that as I
previously was. I'm not certain that const plays into that.

Btw, since I forgot to mention it before, I think this is a great idea
in general, thank you for working on it! :-)

~Aaron