Szelethus added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption<Boolean, "AggressiveStdFindModeling", "Enables exploration of the failure branch in std::find-like " "functions.", "false", Released> ]>, ---------------- Szelethus wrote: > NoQ wrote: > > baloghadamsoftware wrote: > > > Szelethus wrote: > > > > Ah, okay, I see which one you refer to. We should totally make this > > > > non-user facing as well. > > > The option is not about state split! It is for choosing between the > > > (default) conservative approach and a more aggressive one. It is > > > absolutely for the user to set. Some users prefer less false positive for > > > the price of losing true positives. However, some other users prefer more > > > true positives for the price of additional false positives. This is why > > > we have checker options to be able to serve both groups. > > > > > > This feature was explicitly requested by our users who first disabled > > > iterator checkers because of the too many false positives but later > > > re-enabled them because they run into a bug in a production system which > > > could have been prevented by enabling them. However, they run into > > > another bug that our checker did not find because of its conservative > > > behavior. They requested a more aggressive one but we must not do it for > > > everyone. The concept of the Analyzer is that we apply the conservative > > > approach by default and the aggressive can be enabled by analyzer and > > > checker options. > > These options are unlikely to be on-by-default in vanilla clang because of > > having a fundamentally unfixable class of false positives associated with > > them. Honestly, i've never seen users who are actually ok with false > > positives. I've regularly seen users say that they are ok with false > > positives when they wanted their false negatives fixed, but it always > > turned out that they don't understand what they're asking for and they > > quickly changed their mind when they saw the result. But you guys basically > > vend your own tool to your own customers and bear your own responsibility > > and i therefore can't tell you what to do in your realm, and i'm ok with > > having options that allow you to disagree with my choices. > > > > inb4, "//`-analyzer-config` is not user-facing!!!11//" > >inb4, "-analyzer-config is not user-facing!!!11" > I was starting to breath really heavy and type really fast :D > > Jokes aside, I'll spill the beans, we don't have a really convenient way to > tweak these options through CodeChecker just yet, so for the time being, they > really aren't, but maybe one day. > > With that said, I also agree with you regarding options that overapproximate > the analysis (greater code coverage with more false and true positives). User > facing options should be the ones where the nature of the bug itself if > subjective, like whether we want to check the initializedness of pointees. We > sure can regard them as unwanted, or we could regard them as totally fine, so > an option is a great compromise. > > >[...] it always turned out that they don't understand what they're asking > >for [...] > > This is why I think this shouldn't be user-facing. You need to be quite > knowledgeable about the analyzer, and IteratorChecker in particular to make > good judgement about enabling the option added in this patch. That doesn't > mean you shouldn't experiment with it, but I'd prefer to not expose it too > much. > > I just don't see this feature being that desired by many if they knew the > costs, even if one user really wants it. We could however, in that case, say > "Hey, there is this option you can enable that might help you hunt down more > bugs. It does change the actual analysis itself, and experience showed that > it might result in more false positives and could impact performance, so we > opted not to make it user-facing, but it is there, if you feel adventurous." Granted, I might be demonizing the cons of these options too much, but the description doesn't help on understanding it that much either O:) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77150/new/ https://reviews.llvm.org/D77150 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits