rtrieu added a comment. In D130510#3693494 <https://reviews.llvm.org/D130510#3693494>, @aaron.ballman wrote:
> In D130510#3692654 <https://reviews.llvm.org/D130510#3692654>, @rtrieu wrote: > >> 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. Not being able to detect when expressions are dependent inside template instantiations has been a pain for warnings since forever. >> 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. I agree, the CFG should be as broadly applicable as possible, so using an evaluator there is fine. Manually handling every single case may be needed to keep the warning under control. It's possible to put that handling to the Sema side of things, right before the warning is emitted. There's already a filter for macros, so maybe adding the filtering logic there would be a good fix. 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
