tbaeder added a comment.
Getting a review usually takes far longer than just a few hours, so don't ping
too early. I also want an opinion from at least one other reviewer.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:8505
+ // maybe this expression can be evaluated at compile-time
+ // checks if E can be evaluated to some StringLiteral and retry
----------------
The comment should be full sentences, start with an upper-case character and
end in a period:
```
// If this expression can be evaluated at compile-time,
// check if the result is a StringLiteral and retry
```
(just a suggestion)
================
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) {
----------------
tbaeder wrote:
> 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
What about this? I.e. usong `StringLiteral*` directly in the `dyn_cast` call?
================
Comment at: clang/test/Sema/format-strings-scanf.c:236
+ scanf(1 ? "%s" : "%d", i); // expected-warning{{format specifies type 'char
*'}} \
+ // expected-note{{format string is defined here}}
scanf(0 ? "%d %d" : "%d", i); // no warning
----------------
inclyc wrote:
> ```
> // long macro
> #include <cstdio>
>
> #define SOME_STRANGE_MACRO(a, b) ((0) ? (a) : (b))
>
> int main() {
> int *i;
> scanf(SOME_STRANGE_MACRO("%d", "%d %s"), i);
> }
> ```
>
> previous:
> ```
> sample.cpp:7:39: warning: more '%' conversions than data arguments
> [-Wformat-insufficient-args]
> scanf(SOME_STRANGE_MACRO("%d", "%d %s"), i);
> ~^
> sample.cpp:3:48: note: expanded from macro 'SOME_STRANGE_MACRO'
> #define SOME_STRANGE_MACRO(a, b) ((0) ? (a) : (b))
> ^
> 1 warning generated.
> ```
>
> now:
>
> ```
> sample.cpp:7:9: warning: more '%' conversions than data arguments
> [-Wformat-insufficient-args]
> scanf(SOME_STRANGE_MACRO("%d", "%d %s"), i);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sample.cpp:3:34: note: expanded from macro 'SOME_STRANGE_MACRO'
> #define SOME_STRANGE_MACRO(a, b) ((0) ? (a) : (b))
> ^~~~~~~~~~~~~~~~~
> sample.cpp:7:39: note: format string is defined here
> scanf(SOME_STRANGE_MACRO("%d", "%d %s"), i);
> ~^
> sample.cpp:3:48: note: expanded from macro 'SOME_STRANGE_MACRO'
> #define SOME_STRANGE_MACRO(a, b) ((0) ? (a) : (b))
> ^
> 1 warning generated.
> ```
>
> I think it is better to underline the buggy expression, if this sucks maybe
> we can check if this is constexpr **after** checking conditional operator ` ?
> :` or somehow other statement classes ?
I think the output is fine as-is for this patch, but could be improved later.
Looking at this as a human, I feel as if both of the "expanded from macro"
notes are redundant. But I guess they can provide useful information in other
cases.
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
