[PATCH] D94640: adds more checks to -Wfree-nonheap-object

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10252
 namespace {
-void CheckFreeArgumentsOnLvalue(Sema &S, const std::string &CalleeName,
-                                const UnaryOperator *UnaryExpr,
-                                const VarDecl *Var) {
-  StorageClass Class = Var->getStorageClass();
-  if (Class == StorageClass::SC_Extern ||
-      Class == StorageClass::SC_PrivateExtern ||
-      Var->getType()->isReferenceType())
-    return;
-
-  S.Diag(UnaryExpr->getBeginLoc(), diag::warn_free_nonheap_object)
-      << CalleeName << Var;
-}
+constexpr int Placeholder = 0;
 
----------------
cjdb wrote:
> aaron.ballman wrote:
> > After seeing my suggested workaround in practice, I'm hopeful we can figure out how to get the diagnostics engine to not require us to jump through these hoops. I wouldn't block the patch over this approach, but it's not very obvious what's happening from reading the code either.
> Sorry, I'm not entirely sure what you're getting at here? :/
I used to find the `Placeholder` stuff to be confusing in practice and would have preferred to see a diagnostic solution that doesn't need the extra argument. Now I think I'm changing my mind and have a different suggestion -- remove the `Placeholder` and `NoPlaceholder` variables and pass in a literal with a comment:
```
S.Diag(UnaryExpr->getBeginLoc(), diag::warn_free_nonheap_object)
        << CalleeName << 0 /*object*/<< cast<NamedDecl>(D);

  S.Diag(Lambda->getBeginLoc(), diag::warn_free_nonheap_object)
      << CalleeName << 2 /*lambda*/;
```
That removes the akwardness of trying to name the variables, which is what was was I found non-obvious before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94640