Szelethus added a comment.
This checker seems to only check LLVM functions, but doesn't check whether
these methods lie in the LLVM namespace. Is this intended?
================
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.
----------------
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).
```
================
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:
> > 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?
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:210
/// the default tag for the checker will be used.
+ /// @param IsPrunable Whether the note is prunable.
ExplodedNode *
----------------
Charusso wrote:
> It makes perfectly no sense here. Is it the mentioned "class doc"?
You're right. I meant to add it to `NoteTag`s class doc, but whether a
`NoteTag` is prunable isn't a property of `NoteTag` itself, but rather the
infrastructure around it. Oops! I think above `const NoteTag *getNoteTag()`
would be better.
================
Comment at: clang/test/Analysis/return-value-guaranteed.cpp:90
+
+ // no-warning: "The left operand of '==' is a garbage value" was here.
+ doSomething();
----------------
Was it? I just tried it out and it doesn't seem to be the case.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63915/new/
https://reviews.llvm.org/D63915
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
