[PATCH] D121863: [clang][dataflow] Model the behavior of non-standard optional assignment

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:81
 
+auto isOptionalValueOrConversionAssignment() {
+  return cxxOperatorCallExpr(
----------------
While really like the convenience matchers will give us building data flow analyses, I wonder whether a lot of duplicated work is happening in the background. E.g. will we end up string comparing the class name of the parent of the method in the matcher built by `optionalClass` whenever we process an overloaded `operator=`?

In a handwritten implementation we would only do this once, store the address of the canonical declaration somewhere and subsequent checks would only look up the canonical declaration of the called method and do a pointer comparison. 

In case this matcher approach is not that efficient, I wonder if it would be possible to come up with some APIs where the matching is only done once and subsequent transfer functions would only use the cached pointers to declarations/types. In case the matchers are already doing something smart in the background feel free to ignore this comment. 


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:159-160
                         LatticeTransferState &State) {
-  if (auto *OptionalVal = cast_or_null<StructValue>(
-          State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer))) {
+  if (auto *OptionalVal = cast_or_null<StructValue>(State.Env.getValue(
+          *ObjectExpr->IgnoreParens(), SkipPast::ReferenceThenPointer))) {
     auto *HasValueVal = getHasValue(OptionalVal);
----------------
If we never assign values to `Paren`s, shouldn't `getValue` do the skipping instead?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:242
+
+  const int TemplateParamOptionalWrappersCount = countOptionalWrappers(
+      *MatchRes.Context, stripReference(E->getDirectCallee()
----------------
Is this duplicated across here and `transferValueOrConversionConstructor`? Should we have a separate function like `IsValueOperation`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121863