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

[Daniel Marjamäki via cfe-commits]

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.

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.

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

> > 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.

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.

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.



http://reviews.llvm.org/D12359