ymandel added inline comments.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:789
- // of these accessors.
- .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
[](const CallExpr *E,
----------------
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.
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
