aaron.ballman added a comment.
In D130510#3692654 <https://reviews.llvm.org/D130510#3692654>, @rtrieu wrote:
> This warning is now flagging some code which I believe is a false positive.
> In particular, when template arguments are involved, their values can be
> calculated during template instantiation, but they should be treated as
> variables instead of constants. For example:
>
> template <int I, class T>
> void foo(int x) {
> bool b1 = (x & sizeof(T)) == 8;
> bool b2 = (x & I) == 8;
> bool b3 = (x & 4) == 8;
> }
>
> void run(int x) {
> foo<4, int>(8);
> }
>
> In this instantiation, x is and'ed with the value 4 in different ways.
> `sizeof(T)` is type-dependent, `I` is value-dependent, and `4` is an integer
> literal. With your code, each of them would produce a warning. However, the
> first two values of 4 will change in different template instantiations, so
> the warning is not useful when it is okay for some instantiations to have
> constant values.
Yeah, this is definitely a false positive we need to avoid, thank you for
bringing it up!
> Because of this, warnings should treat dependent expressions as non-constant
> even when they can be evaluated, so only `b3` should get a warning. This is
> causing the warning to be emitted on code heavy in template meta-programming,
> such as array libraries. Please revert or fix.
Yeah, I agree. Unfortunately, the CFG makes this exceptionally difficult
because it walks over the instantiated code, so we've lost that the original
expression was dependent by the time we get to checking the binary expressions.
The original code worked by virtue of overfitting to *just* integer literals.
> I believe that evaluating the expression would make this warning too broad
> and would need more testing that what was included here. Only handling
> UnaryOperator with IntegerLiteral sub-expression makes more sense for the
> warning, and adding in any new cases if we find them.
I agree that the warning is too broad right now and that's unintentional.
However, manually handling every single case in the CFG as something special is
fragile and what got us this bug in the first place. We have a constant
expression evaluator (two, actually) and we shouldn't have to reimplement it a
third time in the CFG. However, asking a GSoC mentee to address that is well
beyond the scope of what they should be working on. So for now I'm going to
revert this change, reopen the issue, and we'll discuss the next steps off-list
with Usman.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130510/new/
https://reviews.llvm.org/D130510
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
