chrish_ericsson_atx marked an inline comment as done. chrish_ericsson_atx added a comment.
In D66014#1625733 <https://reviews.llvm.org/D66014#1625733>, @Szelethus wrote: > Oh, btw, thank you for working on this! Glad to help! ================ Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98 + // If cast is implicit LValueToRValue, no conversion is taking place, + // and therefore no range check is needed. Don't analyze further. + if (CE->getCastKind() == CK_LValueToRValue) + return; + ---------------- NoQ wrote: > chrish_ericsson_atx wrote: > > NoQ wrote: > > > If you look at the list of cast kinds, you'll be shocked to see > > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast > > > definitely stands out (because it's the only one that has very little to > > > do with actual casting), i'd still be much more comfortable if we > > > *whitelist* the casts that we check, rather than blacklist them. > > > > > > Can you take a look at the list and find which casts are we actually > > > talking about and hardcode them here? > > I'm happy to cross-check a list; however, I'm not aware of the list you are > > referring to. Would you mind providing a bit more detail? > See **[[ > https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def > | include/clang/AST/OperationKinds.def ]]**. Ah. Okay -- That is a list I've seen before, of course... :) Hmm... regarding whitelisting versus blacklisting... In this case, it seems to me that switching to whitelisting casts that we know we should be checking increases the risk that we would introduce a new bug -- specifically that we'd accidentally leave out cast type that should have been included, which would disable a legitimate check (that some users might already be relying on). By contrast, blacklisting the one cast type we know should *not* be checked adds zero new risk and still fixes this bug. The sense of risk for me comes partly from being fairly new to working on clang AST code -- this is my first change. It strikes me that if I were to feel confident about having the "correct" whitelist, I would have to understand every cast type fairly deeply. Given that I see several cast kinds in that list that I know nothing about, I'm concerned with the level of effort required, and that I still won't be able to fully mitigate the risk. Can you help me understand your rationale for preferring a whitelist in this case? Or, do you feel I've overstated the risk here? I'm not emotionally invested in being proven correct-- just want to learn! :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66014/new/ https://reviews.llvm.org/D66014 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
