Charusso marked 5 inline comments as done.
Charusso added inline comments.
================
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.
----------------
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.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
- const T *lookup(const CallEvent &Call) const {
+ Optional<T> lookup(const CallEvent &Call) const {
// Slow path: linear lookup.
----------------
NoQ wrote:
> Charusso wrote:
> > Szelethus wrote:
> > > Charusso wrote:
> > > > NoQ wrote:
> > > > > I hope `T` never gets too expensive to copy. The ideal return value
> > > > > here is `Optional<const T &>` but i suspect that `llvm::Optional`s
> > > > > don't support this (while C++17 `std::optional`s do). Could you
> > > > > double-check my vague memories here?
> > > > Optional<const T *> is working and used widely, I like that.
> > > Why do we need the optional AND the pointer? How about we just return
> > > with a nullptr if we fail to find the call?
> > `Optional<>` stands for optional values, that is why it is made. @NoQ tried
> > to avoid it, but I believe if someone does not use it for an optional
> > value, that break some kind of unspoken standard.
> Well, that'd be the original code.
>
> > I do not like `Optional<const T *>` anymore.
>
> @Charusso, do you still plan to undo this change?
Well, I am here at 2:1 against `Optional<>`, so I think it is your decision.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+ Optional<bool> IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV,
C);
+ if (!IsInvariantBreak)
+ return;
+
----------------
NoQ wrote:
> This looks flipped to me, should probably say `if (IsInvariantBreak) return;`.
It is the `Optional<>` checking, whether we could obtain the value. I really
wanted to write `!hasValue()`, but no one use that, so it is some kind of
unspoken standard to just `!` it.
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
