[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

[Csaba Dabis via Phabricator via cfe-commits]

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