[PATCH] D102728: [clang][Sema] removes -Wfree-nonheap-object reference param false positive

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10466
     if (isa<VarDecl, FunctionDecl>(D))
-      return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr, D);
+      if (!isa<ParmVarDecl>(D) ||
+          !dyn_cast<ParmVarDecl>(D)->getType()->isReferenceType())
----------------
cjdb wrote:
> dblaikie wrote:
> > It doesn't look like the parameter case is tested, is it? And is it necessary? I think it'd be reasonable to warn on `void f1(int i) { free(&i); }` - so I think it's only the "is it a reference type" that's the important property.
> Right, but it's also reasonable to warn on this, which isn't a parameter.
> ```
> int i = 0;
> int& r = i;
> std::free(&r);
> ```
> However, I will take you up on making sure that parameters are tested.
Catching this case seems unlikely to be feasible in a compiler diagnostic - because differentiating the false positives and true positives would require more complicated analysis than is usually done.

eg: We shouldn't warn on this:
```
int *i = (int*)std::malloc(sizeof(int));
int &j = *i;
std::free(&j);
```


================
Comment at: clang/test/Sema/warn-free-nonheap-object.cpp:13-16
+void free_reference(char &x) { ::free(&x); }
+void free_reference(char &&x) { ::free(&x); }
+void std_free_reference(char &x) { std::free(&x); }
+void std_free_reference(char &&x) { std::free(&x); }
----------------
I probably wouldn't bother testing the combinatorial expansion of all options (std versus global, rvalue versus lvalue) - if the features are treated orthogonally in the code (there's little chance that some particular combination would have a problem if each separately didn't have a problem).

& generally I don't think there's any need to have callers of these functions in any context (member function or otherwise - as mentioned further down) - clang warnings generally aren't interprocedural, so there's no real risk that checking the function alone would result in different results than checking it with a caller as well.


================
Comment at: clang/test/Sema/warn-free-nonheap-object.cpp:69-70
+  {
+    char* P = (char *)std::malloc(2);
+    free_reference(*P);
+  }
----------------
dblaikie wrote:
> I don't think this code is necessary - since clang warnings aren't interprocedural, there's no need to flesh out the example this far - you can have the function standalone elsewhere, that takes a reference parameter and tries to free it after taking its address. Without any caller.
^ ping on this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102728/new/

https://reviews.llvm.org/D102728