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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10267-10277
+  if (const auto *Field = dyn_cast<FieldDecl>(D)) {
     S.Diag(UnaryExpr->getBeginLoc(), diag::warn_free_nonheap_object)
         << CalleeName << Field;
+    return;
+  }
+
+  if (const auto *Func = dyn_cast<FunctionDecl>(D)) {
----------------
aaron.ballman wrote:
> I think this simplifies things a bit (even though it's doing an `isa<>` followed by a `cast<>`).
Wow, that's much nicer, thanks! :D


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10286-10289
     if (const auto *Var = dyn_cast<VarDecl>(Lvalue->getDecl()))
       return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr, Var);
+    if (const auto *Var = dyn_cast<FunctionDecl>(Lvalue->getDecl()))
+      return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr, Var);
----------------
aaron.ballman wrote:
> 
This one required a bit more work than the previous one because there are two overloads for `CheckFreeArgumentsOnLvalue`.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10312
+                            const CastExpr *Cast) {
+  auto const kind = Cast->getCastKind();
+  switch (kind) {
----------------
aaron.ballman wrote:
> Please spell out the type (also, we don't typically use top-level `const` qualification on local variables or parameters).
Heh, this was only named for debugging purposes. I've consolidated it into the switch statement ;-)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10314
+  switch (kind) {
+  case clang::CK_IntegralToPointer: // [[fallthrough]];
+  case clang::CK_FunctionToPointerDecay: {
----------------
aaron.ballman wrote:
> 
Done, but why? I quite like making it clear that fallthrough is intentional.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10337
+  // Prefer something that doesn't involve a cast to make things simpler.
+  {
+    const Expr *Arg = E->getArg(0)->IgnoreParenCasts();
----------------
aaron.ballman wrote:
> The extra compound scope doesn't add too much, so I'd remove it.
I find it improves readability and groups what the comment above it (now inline with `{`) is talking about.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10351
+
+    if (const auto *Block = dyn_cast<BlockExpr>(Arg)) {
+      Diag(Block->getBeginLoc(), diag::warn_free_nonheap_object)
----------------
aaron.ballman wrote:
> Any reason not to handle `LambdaExpr` at the same time?
None, I hadn't considered it. I'm not sure I see the relationship bet


================
Comment at: clang/test/Analysis/free.c:84
+  // expected-warning at -1{{Argument to free() is a block, which is not memory allocated by malloc()}}
+  // expected-warning at -2{{attempt to call free on non-heap object : block expression}}
 }
----------------
aaron.ballman wrote:
> The formatting for this diagnostic is somewhat unfortunate in that it has the leading space before the `:`. I think that changing the diagnostic to use a `%select` would be an improvement.
I'm having a *lot* of difficulty getting `%select` to work. Here's what I've tried, but the space in `%select{ %2` is being ignored :(

```
: Warning<"attempt to call %0 on non-heap object%select{ %2|: block expression}1">,
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94640