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

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In http://reviews.llvm.org/D12359#233152, @sberg wrote:

> causes false positive for
>
>   char * f(char *);
>   char * g(char * p) { return f(p); }
>


Sorry for replying this late. This should work in latest patch.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+  InGroup<NonConstParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
----------------
aaron.ballman wrote:
> > I disagree about this. Normal usage is to enable as much warnings as you can.
> > 
> > Is it possible for you to show a document, discussion or something that backs your claim?
> 
> Searching through the Clang email archives yields:
> 
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150504/128373.html
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140922/115379.html
> 
> and others as well. This has been the de facto bar for as long as I've been contributing.
ok thanks for looking it up. I will try to fix all the test cases.

================
Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  if (auto *B = dyn_cast<BinaryOperator>(ER.get())) {
+    if (B->isAssignmentOp() || B->isAdditiveOp()) {
+      MarkNonConstUse(B->getLHS());
----------------
aaron.ballman wrote:
> > basic idea is that p can't be const here:
> ```
> void f(int *p) {
>     int *q = p + 1;
>     // ...
> }
> ```
> But it could be const here:
> ```
> void f(int *p) {
>   const *q = p + 1;
> }
> ```
> I am not certain that addition, by itself, is sufficient to say the use is non-const. At the least, this could have some comments explaining the rationale with a FIXME.
that is not by intention. There is no nonconst use if lhs is a const pointer. I will investigate.

================
Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  Expr *E = ER.get()->IgnoreParenCasts();
+  if (auto *B = dyn_cast<BinaryOperator>(E)) {
+    if (B->isAssignmentOp()) {
----------------
it's handled better now.

================
Comment at: lib/Parse/ParseExpr.cpp:181
@@ +180,3 @@
+             isa<BinaryConditionalOperator>(E) ||
+             isa<ConditionalOperator>(E) ||
+             isa<UnaryOperator>(E)) {
----------------
yes.

dontwarn9 was just a test.. if a nonconst pointer is returned then the parameter can't be const. I have corrected the test, see return6 .

I will add a new test in the next iteration when a const pointer is returned.

================
Comment at: lib/Parse/ParseStmt.cpp:376
@@ +375,3 @@
+// Mark symbols in r-value expression as written.
+void Parser::MarkNonConstUse(Expr *E) {
+  E = E->IgnoreParenCasts();
----------------
aaron.ballman wrote:
> > This is called from the Parser only.
> 
> So will this still properly diagnose the same cases from a serialized AST?
I don't know what this "serialized AST" is that you are talking about. All I know is the -ast-dump and that flag is only intended as a debugging aid as far as I know. If I just run "clang -cc1 -Wnonconst-parameter somefile.c" then it does not serialize does it? So what flags do I use to serialize etc?

I would appreciate if you can show me a command that will cause FP. So I can look at it.

I believe that we can't put this in the Sema only. The parser knows better how the result is used and can set "nonconstuse" properly for expressions.

For instance I don't want to mark all pointer additions as "nonconstuse" just because some of them are "nonconstuse".



http://reviews.llvm.org/D12359