Charusso added a comment.
In D63915#1563049 <https://reviews.llvm.org/D63915#1563049>, @NoQ wrote:
> Aha, nice, thanks for adding a description, it is a very good thing to do.
> Like, every commit should be obvious.
In some of my patches I have not added a description because they are so tiny
changes.
> I guess this checker should eventually be merged with
> `StdCLibraryFunctionsChecker` and such.
Do you have any plans for that?
================
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:
> 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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98
+ SVal RetV = CE.getReturnValue();
+ Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+
----------------
NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > `castAs` if you're sure.
> > >
> > > Generally it's either evaluated conservatively (so the return value is a
> > > conjured symbol) or inlined (and in this case returning an undefined
> > > value would result in a fatal warning), so return values shouldn't ever
> > > be undefined.
> > >
> > > The only way to obtain an undefined return value is by binding it in
> > > `evalCall()`, but even then, i'd rather issue a warning immediately.
> > Sometimes it failed with live tests in the wild.
> I'm mildly interested, can you share a repro if you happen to still have one?
After the patch finishes I will make tests and try to break it again.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:39
+ // 'Error()'
+ {{{"ARMAsmParser", "Error"}}, true},
+ {{{"HexagonAsmParser", "Error"}}, true},
----------------
NoQ wrote:
> We can play even more nicely here by requiring namespace `llvm` as well (just
> prepend one more item to the list).
Well, I have checked 4 of them, they are in the anonymous namespace, so I will
leave it.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82
+
+ Out << '\'' << Name << "' always returns "
+ << (*Value ? "true" : "false");
----------------
NoQ wrote:
> Let's mention the class name as well! Maybe even the fully qualified
> namespace.
The class::call part would be tricky, because you need to hook what do you have
in the CallDescription. It could be done with the decl-matching part, but then
you have to rewrite the CallDescriptionMap interface as `lookup(), key(),
value()`, so you could use the hooked info everywhere. It would require too
much overhead for a print.
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
