tbaeder added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:8507
+ if (Result.Val.isLValue()) {
+ auto *LVE = Result.Val.getLValueBase().dyn_cast<const Expr *>();
+ if (LVE && LVE->getStmtClass() == Stmt::StringLiteralClass) {
----------------
inclyc wrote:
> tbaeder wrote:
> > I think you should be able to unify the two `if` statements.
> >
> > Can you not `dyn_cast_or_null<StringLiteral>(Result.Val.getLValueBase())`
> > here instead of casting to `Expr*` and checking the `StmtClass`?
> `LValueBase` seems to be a wrapper of a pointer that has its own dyn_cast
> method.
>
> I have changed code here like this, but it cannot compile
> ```
> auto *MaybeStringLiteral =
> dyn_cast_or_null<StringLiteral *>(Result.Val.getLValueBase());
> if (MaybeStringLiteral) {
> return checkFormatStringExpr(S, MaybeStringLiteral, Args, APK,
> format_idx,
> firstDataArg, Type, CallType,
> /*InFunctionCall*/ false, CheckedVarArgs,
> UncoveredArg, Offset);
> }
> ```
>
> ```
> <workspace>/llvm-project/llvm/include/llvm/Support/Casting.h:64:53: error:
> type 'clang::StringLiteral *' cannot be used prior to '::' because it has no
> members
> static inline bool doit(const From &Val) { return To::classof(&Val); }
> ```
Ah, shame. This ends up calling `dyn_cast` on a `llvm::Pointerunion` as far as
I can see, which seems to return `nullptr` in case the cast is unsuccessful
anyway, so you could try just exploiting that:
https://llvm.org/doxygen/classllvm_1_1PointerUnion.html#a9147352f78d98ee246f25d3300e0aaba
================
Comment at: clang/test/Sema/format-strings-scanf.c:235
scanf(0 ? "%s" : "%d", i); // no warning
- scanf(1 ? "%s" : "%d", i); // expected-warning{{format specifies type 'char
*'}}
+ scanf(1 ? "%s" : "%d", i); // expected-warning{{format specifies type 'char
*'}} \
+ // expected-note{{format string is defined here}}
----------------
inclyc wrote:
> tbaeder wrote:
> > inclyc wrote:
> > > These new notes are FixIt hints, looks much better than before.
> > Can you show some sample output?
> Source:
> ```
> // sample.cpp
> #include <cstdio>
>
> int main() {
> int *i;
> scanf(1 ? "%s %d" : "%d", i);
> }
> ```
>
> previous version:
> ```
> sample.cpp:5:29: warning: format specifies type 'char *' but the argument has
> type 'int *' [-Wformat]
> scanf(1 ? "%s %d" : "%d", i);
> ~~ ^
> %d
> sample.cpp:5:18: warning: more '%' conversions than data arguments
> [-Wformat-insufficient-args]
> scanf(1 ? "%s %d" : "%d", i);
> ~^
> 2 warnings generated.
> ```
>
> this patch highlights ` cond ? T : F` expressions:
>
> ```
> sample.cpp:5:29: warning: format specifies type 'char *' but the argument has
> type 'int *' [-Wformat]
> scanf(1 ? "%s %d" : "%d", i);
> ~~~~~~~~~~~~~~~~~~ ^
> sample.cpp:5:14: note: format string is defined here
> scanf(1 ? "%s %d" : "%d", i);
> ^~
> %d
> sample.cpp:5:9: warning: more '%' conversions than data arguments
> [-Wformat-insufficient-args]
> scanf(1 ? "%s %d" : "%d", i);
> ^~~~~~~~~~~~~~~~~~
> sample.cpp:5:18: note: format string is defined here
> scanf(1 ? "%s %d" : "%d", i);
> ~^
> 2 warnings generated.
> ```
>
Alright, might be a bit verbose but I agree that it is useful.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130906/new/
https://reviews.llvm.org/D130906
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
