[PATCH] D150775: [clang][dataflow] Fix a bug in handling of `operator->` for optional checker.

Wed May 17 07:54:10 PDT 2023

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