[PATCH] D122231: [clang][dataflow] Add support for `value_or` in a comparison.

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

xazax.hun added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:142
+  // `opt.value_or(nullptr) != nullptr` and `opt.value_or(0) != 0`. Ideally,
+  // we'd support this pattern for any expression, but the AST does not have a
+  // generic expression comparison facility, so we specialize to common cases
----------------
Yeah, I think Clang is in a very sad state in this regard. We have a lot of half done facilities littered all over the codebase, including:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Analysis/CloneDetection.h
https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp#L306
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp#L60




================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:147
+                         anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
+                               ComparesToSame(integerLiteral(equals(0)))));
+}
----------------
I wonder if we want to add `""` to support `opt.value_or("") != ""`. Not sure how frequent would this be over the empty call.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:270
+  // needed.
+  BoolValue &ComparisonValue = MakeValue(Env, *HasValueVal);
+  auto *ComparisonExprLoc =
----------------
Is this the right way to initialize `ComparisonValue`?

Considering the expression: `opt.value_or(nullptr) != nullptr`
* When `has_value == false`, `opt.value_or(nullptr)` will return `nullptr`, so `!=` evaluates to false. This case seems to check out.
* However, when `has_value == true`, `opt` might still hold an `nullptr` and `!=` could still evaluate to false. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122231