Charusso added a comment. Thanks for the reviews! The remaining question is: do we want to use `Optional<>` in the `CallDescriptionMap::lookup()`?
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled by the target triple. ---------------- Szelethus wrote: > Charusso wrote: > > NoQ wrote: > > > Charusso wrote: > > > > NoQ wrote: > > > > > Szelethus wrote: > > > > > > Charusso wrote: > > > > > > > Szelethus wrote: > > > > > > > > This isn't true: the user may decide to only enable > > > > > > > > non-pathsensitive checkers. > > > > > > > > > > > > > > > > I think the comment should rather state that these whether > > > > > > > > these checkers are enabled shouldn't be explicitly specified, > > > > > > > > unless in **extreme** circumstances (causes a crash in a > > > > > > > > release build?). > > > > > > > Well, I have removed it instead. Makes no sense, you are right. > > > > > > I don't think it's a good idea -- we definitely should eventually > > > > > > be able to list packages with descriptions just like checkers > > > > > > (there actually is a FIXME in CheckerRegistry.cpp for that), but > > > > > > this is the next best thing that we have. > > > > > > > > > > > > How about this: > > > > > > ``` > > > > > > // The APIModeling package is for checkers that model APIs and > > > > > > don't perform > > > > > > // any diagnostics. Checkers within this package are enabled by the > > > > > > core or > > > > > > // through checker dependencies, so one shouldn't enable/disable > > > > > > them by > > > > > > // hand (unless they cause a crash, but that will cause dependent > > > > > > checkers to be > > > > > > // implicitly disabled). > > > > > > ``` > > > > > I don't think any of these are dependencies. Most of the > > > > > `apiModeling` checkers are there to suppress infeasible paths > > > > > (exactly like this one). > > > > > > > > > > I think i'd prefer to leave the comment as-is. We can always update > > > > > it later. > > > > Thanks! Copy-pasted, just that patch produce diagnostics as notes. > > > Let's change to `don't emit any warnings` then. > > I think an APIModeling could not be too much thing, most of the stuff > > around it is not commented out what they do. But as @Szelethus really > > wanted to inject that, I cannot say no to a copy-paste. > Some are, but saying something along the lines of "most of these are enabled > by default by the Driver" would've been more correct, but yea, let's leave > this for another day. Well, okay. Something like that is supposed to be correct for future developments: ``` // Checkers within APIModeling package are model APIs and enabled by the core // or through checker dependencies, so one should not enable/disable them by // hand (unless they cause a crash, but that will cause dependent checkers to // be implicitly disabled). // They do not emit any warnings, just suppress infeasible paths. ``` ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:88-93 + Optional<const bool *> RawExpectedValue = CDM.lookup(Call); + if (!RawExpectedValue) + return; + + SVal ReturnV = Call.getReturnValue(); + bool ExpectedValue = **RawExpectedValue; ---------------- NoQ wrote: > This can still be re-used by moving into `isInvariantBreak` (you can give it > access to `CDM` by making it non-static). Well, sadly not. In both of the checks it is used inside the call, so you cannot just remove it. I really wanted to make it as simple as possible, but you know, "not simpler". ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:14 + +#include "clang/Driver/DriverDiagnostic.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" ---------------- xazax.hun wrote: > Is DriverDiagnostic used for something? I thought, but definitely not, good catch! ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:69 + Name += Call.getCalleeIdentifier()->getName(); + return Name; +} ---------------- xazax.hun wrote: > `CXXMethodDecl::getQualifiedNameAsString` is not doing what you want here? We want to drop the namespaces for better readability. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
