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@-1{{Argument to free() is a block, which is not memory
allocated by malloc()}}
+ // expected-warning@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
