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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
