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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb marked an inline comment as done.
cjdb added a comment.

In D94640#2579447 <https://reviews.llvm.org/D94640#2579447>, @aaron.ballman wrote:

> In D94640#2560647 <https://reviews.llvm.org/D94640#2560647>, @cjdb wrote:
>
>> fixes block expressions
>>
>> @aaron.ballman I'm not sure I follow what you're after re lambdas. Could you please provide a test case that I can work with?
>
> Sorry about missing your question -- this fell off my radar. I was thinking it'd be roughly the same as blocks, but I now think this'll create compile errors anyway: `free([]{ return nullptr; });` (where the lambda is accidentally not being called). So not nearly as important as I was thinking (sorry for that, but thank you for adding the support and test cases just the same!).
>
> One worry I have is the number of effectively duplicated diagnostics we're now getting in conjunction with the static analyzer. It's not quite perfect overlap and so we can't get rid of the static analyzer check, but is there a long-term plan for how to resolve this?

I filed issue 48767 <https://bugs.llvm.org/show_bug.cgi?id=48767>, but I don't think the analyser folks are motivated to address it any time soon.



================
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;
 
----------------
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? :/


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10342
+      static constexpr int NoPlaceholder = 1;
+      Diag(Arg->getBeginLoc(), diag::warn_free_nonheap_object)
+          << CalleeName << NoPlaceholder;
----------------
aaron.ballman wrote:
> FWIW, this diagnostic will be awkward for lambdas because it mentions blocks explicitly. It's not critical thing given how hard it would be to hit the lambda case, but if you think of an easy way to solve it, I won't complain.
Nothing "easy" comes to mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94640