NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
I think i like it now!
================
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.
----------------
Charusso wrote:
> 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.
I'd rather leave the original code as is and think more deeply about adding
support for `Optional<const T &>` in the future.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:229
+ /// @param Cb Callback with 'BugReporterContext &, BugReport &' parameters.
+ /// @param IsPrunable Whether the note is prunable.
+ const NoteTag *getNoteTag(NoteTag::Callback &&Cb, bool IsPrunable = false) {
----------------
"Allow BugReporter to omit the note from the report if it would make the
displayed bug path significantly shorter."
================
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;
----------------
Charusso wrote:
> 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".
Aha, ok, right!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+ Optional<bool> IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV,
C);
+ if (!IsInvariantBreak)
+ return;
+
----------------
Charusso wrote:
> 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.
Mmm. Aha. Ok. Indeed. Sry^^
I was thinking about simply err-ing towards "it's not a break" when we don't
know for sure that it's a break, because in this case we have no problems with
taking the branch that we want.
But your code seems to be more careful and i like it :)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:69
+ Name += Call.getCalleeIdentifier()->getName();
+ return Name;
+}
----------------
Charusso wrote:
> xazax.hun wrote:
> > `CXXMethodDecl::getQualifiedNameAsString` is not doing what you want here?
> We want to drop the namespaces for better readability.
Yeah, i think it's important to display exactly what we match for.
We might eventually do some sort of `CallDescription.explain()` (and then maybe
even `CallDescriptionMap.explain(Call)`) for that purpose.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:154
+
+ BR.markInteresting(SFC);
+
----------------
Hmm. It is enough to set `IsPrunable` to `false`; once you do that, there's
actually no need to mark the stack frame as interesting. I guess this wasn't
really necessary.
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
