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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
