Szelethus added a comment.
This checker isn't in alpha -- did you evaluate it on LLVM? Other than that,
looks great!
================
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:
> > 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.
Are you aware of this thread?
http://lists.llvm.org/pipermail/cfe-dev/2018-July/058427.html
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
