ymandel created this revision.
ymandel added reviewers: xazax.hun, gribozavr2.
Herald added subscribers: martong, rnkovacs.
Herald added a reviewer: NoQ.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
Previously, in the case of an optional constructed from `nullopt`, we relied on
the value constructed for the `nullopt`. This complicates the implementation and
exposes it to bugs (indeed, one such was found), yet doesn't improve the
engine. Instead, this patch constructs a fresh optional representation, rather
than relying on the underlying nullopt representation.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D140506
Files:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1273,7 +1273,6 @@
ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"));
}
-private:
template <typename FuncDeclMatcher>
void ExpectDiagnosticsFor(std::string SourceCode,
FuncDeclMatcher FuncMatcher) {
@@ -2939,6 +2938,38 @@
)");
}
+TEST_P(UncheckedOptionalAccessTest, CtorInitializerNullopt) {
+ using namespace ast_matchers;
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Target {
+ Target(): opt($ns::nullopt) {
+ opt.value(); // [[unsafe]]
+ }
+ $ns::$optional<int> opt;
+ };
+ )",
+ cxxConstructorDecl(ofClass(hasName("Target"))));
+}
+
+TEST_P(UncheckedOptionalAccessTest, CtorInitializerValue) {
+ using namespace ast_matchers;
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Target {
+ Target(): opt(3) {
+ opt.value();
+ }
+ $ns::$optional<int> opt;
+ };
+ )",
+ cxxConstructorDecl(ofClass(hasName("Target"))));
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -438,12 +438,11 @@
Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
}
-void assignOptionalValue(const Expr &E, LatticeTransferState &State,
+void assignOptionalValue(const Expr &E, Environment &Env,
BoolValue &HasValueVal) {
if (auto *OptionalLoc =
- State.Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) {
- State.Env.setValue(*OptionalLoc,
- createOptionalValue(State.Env, HasValueVal));
+ Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) {
+ Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal));
}
}
@@ -479,7 +478,7 @@
LatticeTransferState &State) {
assert(E->getNumArgs() > 0);
- assignOptionalValue(*E, State,
+ assignOptionalValue(*E, State.Env,
valueOrConversionHasValue(*E->getConstructor(),
*E->getArg(0), MatchRes,
State));
@@ -647,35 +646,35 @@
// make_optional
.CaseOfCFGStmt<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
- // optional::optional
+ // optional::optional (in place)
.CaseOfCFGStmt<CXXConstructExpr>(
isOptionalInPlaceConstructor(),
[](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(true));
+ assignOptionalValue(*E, State.Env,
+ State.Env.getBoolLiteralValue(true));
})
+ // nullopt_t::nullopt_t
.CaseOfCFGStmt<CXXConstructExpr>(
isNulloptConstructor(),
[](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E, State,
+ assignOptionalValue(*E, State.Env,
State.Env.getBoolLiteralValue(false));
})
+ // optional::optional(nullopt_t)
.CaseOfCFGStmt<CXXConstructExpr>(
isOptionalNulloptConstructor(),
[](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- // Shares a temporary with the underlying `nullopt_t` instance.
- if (auto *OptionalLoc =
- State.Env.getStorageLocation(*E, SkipPast::None)) {
- State.Env.setValue(
- *OptionalLoc,
- *State.Env.getValue(*E->getArg(0), SkipPast::None));
- }
+ assignOptionalValue(*E, State.Env,
+ State.Env.getBoolLiteralValue(false));
})
+ // optional::optional (value/conversion)
.CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
transferValueOrConversionConstructor)
+
// optional::operator=
.CaseOfCFGStmt<CXXOperatorCallExpr>(
isOptionalValueOrConversionAssignment(),
@@ -714,7 +713,7 @@
isOptionalMemberCallWithName("emplace"),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E->getImplicitObjectArgument(), State,
+ assignOptionalValue(*E->getImplicitObjectArgument(), State.Env,
State.Env.getBoolLiteralValue(true));
})
@@ -723,7 +722,7 @@
isOptionalMemberCallWithName("reset"),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E->getImplicitObjectArgument(), State,
+ assignOptionalValue(*E->getImplicitObjectArgument(), State.Env,
State.Env.getBoolLiteralValue(false));
})
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
