mboehme marked 2 inline comments as done.
mboehme added inline comments.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:789
- // of these accessors.
- .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
[](const CallExpr *E,
----------------
ymandel wrote:
> mboehme wrote:
> > ymandel wrote:
> > > this function will be left with only one caller. Maybe inline it? For
> > > that matter, should the other caller be changed in some parallel way?
> > > this function will be left with only one caller. Maybe inline it?
> >
> > The general style in this file seems to be to pull matcher expressions out
> > into helper functions, even if they're only used in one place. FWIW, I'm
> > personally not convinced this is helpful because you _want_ to be able to
> > see exactly what a `CaseOfCFGStmt` is matching on, so inlining makes sense
> > IMO. But I'm not sure I want to be inconsistent with the general style of
> > this file -- it would make more sense to do a general cleanup. WDYT?
> >
> > > For that matter, should the other caller be changed in some parallel way?
> >
> > No, that caller (`buildDiagnoseMatchSwitch()`) does not have the same
> > problem because it looks only at the `optional`, not at the value that is
> > contained in it (see also [D150756](https://reviews.llvm.org/D150756),
> > which makes this explicit), and the distinction between `operator*` and
> > `operator->` (for our purposes) is only in the way they return the
> > contained value.
> regarding the matcher -- it's a judgment call; I agree that having to jump
> back and forth to read this case "statement" is problematic, but some of the
> matchers are kind of complicated, so a name can be a service to the reader.
> This one looked sufficiently simple to consider just inlining it, but I'm
> also fine leaving as is.
In that case, I think I'll leave this as-is for the time being and will revisit
the question of inlining more generally.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150775/new/
https://reviews.llvm.org/D150775
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
