[PATCH] D148344: [clang][dataflow] Refine matching of optional types to anchor at top level.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:262
 /// Returns true if and only if `Type` is an optional type.
 bool isOptionalType(QualType Type) {
   if (!Type->isRecordType())
----------------
ymandel wrote:
> ymandel wrote:
> > xazax.hun wrote:
> > > Why do we need two places to define what is an optional type? Would it be possible to somehow reuse the Declaration matcher on the RecordDecel here? Or would that be bad for performance?
> > Good point. I dismissed reusing the matcher here because of performance concerns -- setting up the match is expensive (in the constant-time sense but still a lot of overhead). But, we're already using matching quite a lot so I suspect it doesn't matter. Still, what about the reverse -- use this predicate in the matcher, which is faster while still ensuring consistent results?
> > 
> > That said, what I really want is for the matcher logic to be made available outside of the matcher framework -- I think `NamedDecl` should have the core logic (parallel to `getQualifiedNameAsString`) and that could be used elsewhere, including the matcher.
> regardless, the matcher is wrong (if we keep it): it should be `hasAnyName` rather than `anyOf` and lots of `hasName`. not sure how I never caught that earlier...
I've updated the matcher but otherwise left the code alone. I will send a followup patch that shares the code now that I understand how to avoid the mention of the libc++ internal names.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148344/new/

https://reviews.llvm.org/D148344