Charusso added a comment.
In D63915#1568166 <https://reviews.llvm.org/D63915#1568166>, @Szelethus wrote:
> This checker seems to only check LLVM functions, but doesn't check whether
> these methods lie in the LLVM namespace. Is this intended?
Thanks for the reviews! They are not in the `llvm` namespace.
================
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:
> 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.
================
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.
----------------
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.
================
Comment at: clang/test/Analysis/return-value-guaranteed.cpp:90
+
+ // no-warning: "The left operand of '==' is a garbage value" was here.
+ doSomething();
----------------
Szelethus wrote:
> Was it? I just tried it out and it doesn't seem to be the case.
Whoops, too heavy copy-pasting.
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
