mboehme updated this revision to Diff 510496.
mboehme marked an inline comment as done.
mboehme added a comment.
Changes in response to review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147302/new/
https://reviews.llvm.org/D147302
Files:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -312,8 +312,7 @@
if (Member->getType()->isReferenceType()) {
auto &MemberLoc = ThisLoc.getChild(*Member);
- Env.setValue(MemberLoc, Env.takeOwnership(std::make_unique<ReferenceValue>(
- *InitStmtLoc)));
+ Env.setValue(MemberLoc, Env.create<ReferenceValue>(*InitStmtLoc));
} else {
auto &MemberLoc = ThisLoc.getChild(*Member);
Env.setValue(MemberLoc, *InitStmtVal);
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -237,7 +237,7 @@
Env.setStorageLocation(*S, *DeclLoc);
} else {
auto &Loc = Env.createStorageLocation(*S);
- auto &Val = Env.takeOwnership(std::make_unique<ReferenceValue>(*DeclLoc));
+ auto &Val = Env.create<ReferenceValue>(*DeclLoc);
Env.setStorageLocation(*S, Loc);
Env.setValue(Loc, Val);
}
@@ -276,8 +276,7 @@
// FIXME: reuse the ReferenceValue instead of creating a new one.
if (auto *InitExprLoc =
Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
- auto &Val =
- Env.takeOwnership(std::make_unique<ReferenceValue>(*InitExprLoc));
+ auto &Val = Env.create<ReferenceValue>(*InitExprLoc);
Env.setValue(Loc, Val);
}
} else if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
@@ -423,8 +422,8 @@
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- Env.setValue(Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(
- SubExprVal->getPointeeLoc())));
+ Env.setValue(Loc,
+ Env.create<ReferenceValue>(SubExprVal->getPointeeLoc()));
break;
}
case UO_AddrOf: {
@@ -437,8 +436,7 @@
break;
auto &PointerLoc = Env.createStorageLocation(*S);
- auto &PointerVal =
- Env.takeOwnership(std::make_unique<PointerValue>(*PointeeLoc));
+ auto &PointerVal = Env.create<PointerValue>(*PointeeLoc);
Env.setStorageLocation(*S, PointerLoc);
Env.setValue(PointerLoc, PointerVal);
break;
@@ -468,8 +466,7 @@
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- Env.setValue(Loc, Env.takeOwnership(
- std::make_unique<PointerValue>(*ThisPointeeLoc)));
+ Env.setValue(Loc, Env.create<PointerValue>(*ThisPointeeLoc));
}
void VisitReturnStmt(const ReturnStmt *S) {
@@ -523,8 +520,7 @@
} else {
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- Env.setValue(Loc, Env.takeOwnership(
- std::make_unique<ReferenceValue>(*VarDeclLoc)));
+ Env.setValue(Loc, Env.create<ReferenceValue>(*VarDeclLoc));
}
return;
}
@@ -558,8 +554,7 @@
} else {
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
- Env.setValue(
- Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(MemberLoc)));
+ Env.setValue(Loc, Env.create<ReferenceValue>(MemberLoc));
}
}
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -221,9 +221,9 @@
/// Creates a symbolic value for an `optional` value using `HasValueVal` as the
/// symbolic value of its "has_value" property.
StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
- auto OptionalVal = std::make_unique<StructValue>();
- setHasValue(*OptionalVal, HasValueVal);
- return Env.takeOwnership(std::move(OptionalVal));
+ auto &OptionalVal = Env.create<StructValue>();
+ setHasValue(OptionalVal, HasValueVal);
+ return OptionalVal;
}
/// Returns the symbolic value that represents the "has_value" property of the
@@ -312,8 +312,8 @@
return nullptr;
auto &ValueLoc = Env.createStorageLocation(Ty);
Env.setValue(ValueLoc, *ValueVal);
- auto ValueRef = std::make_unique<ReferenceValue>(ValueLoc);
- OptionalVal.setProperty("value", Env.takeOwnership(std::move(ValueRef)));
+ auto &ValueRef = Env.create<ReferenceValue>(ValueLoc);
+ OptionalVal.setProperty("value", ValueRef);
return &ValueLoc;
}
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -376,7 +376,7 @@
QualType ParamType = Param->getType();
if (ParamType->isReferenceType()) {
- auto &Val = takeOwnership(std::make_unique<ReferenceValue>(*ArgLoc));
+ auto &Val = create<ReferenceValue>(*ArgLoc);
setValue(Loc, Val);
} else if (auto *ArgVal = getValue(*ArgLoc)) {
setValue(Loc, *ArgVal);
@@ -702,7 +702,7 @@
// with integers, and so distinguishing them serves no purpose, but could
// prevent convergence.
CreatedValuesCount++;
- return &takeOwnership(std::make_unique<IntegerValue>());
+ return &create<IntegerValue>();
}
if (Type->isReferenceType()) {
@@ -719,7 +719,7 @@
setValue(PointeeLoc, *PointeeVal);
}
- return &takeOwnership(std::make_unique<ReferenceValue>(PointeeLoc));
+ return &create<ReferenceValue>(PointeeLoc);
}
if (Type->isPointerType()) {
@@ -736,7 +736,7 @@
setValue(PointeeLoc, *PointeeVal);
}
- return &takeOwnership(std::make_unique<PointerValue>(PointeeLoc));
+ return &create<PointerValue>(PointeeLoc);
}
if (Type->isStructureOrClassType() || Type->isUnionType()) {
@@ -756,8 +756,7 @@
Visited.erase(FieldType.getCanonicalType());
}
- return &takeOwnership(
- std::make_unique<StructValue>(std::move(FieldValues)));
+ return &create<StructValue>(std::move(FieldValues));
}
return nullptr;
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -59,10 +59,9 @@
: getReferencedFields(Type);
for (const FieldDecl *Field : Fields)
FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
- return takeOwnership(
- std::make_unique<AggregateStorageLocation>(Type, std::move(FieldLocs)));
+ return create<AggregateStorageLocation>(Type, std::move(FieldLocs));
}
- return takeOwnership(std::make_unique<ScalarStorageLocation>(Type));
+ return create<ScalarStorageLocation>(Type);
}
StorageLocation &
@@ -90,8 +89,7 @@
auto Res = NullPointerVals.try_emplace(CanonicalPointeeType, nullptr);
if (Res.second) {
auto &PointeeLoc = createStorageLocation(CanonicalPointeeType);
- Res.first->second =
- &takeOwnership(std::make_unique<PointerValue>(PointeeLoc));
+ Res.first->second = &create<PointerValue>(PointeeLoc);
}
return *Res.first->second;
}
@@ -112,8 +110,7 @@
auto Res = ConjunctionVals.try_emplace(makeCanonicalBoolValuePair(LHS, RHS),
nullptr);
if (Res.second)
- Res.first->second =
- &takeOwnership(std::make_unique<ConjunctionValue>(LHS, RHS));
+ Res.first->second = &create<ConjunctionValue>(LHS, RHS);
return *Res.first->second;
}
@@ -125,15 +122,14 @@
auto Res = DisjunctionVals.try_emplace(makeCanonicalBoolValuePair(LHS, RHS),
nullptr);
if (Res.second)
- Res.first->second =
- &takeOwnership(std::make_unique<DisjunctionValue>(LHS, RHS));
+ Res.first->second = &create<DisjunctionValue>(LHS, RHS);
return *Res.first->second;
}
BoolValue &DataflowAnalysisContext::getOrCreateNegation(BoolValue &Val) {
auto Res = NegationVals.try_emplace(&Val, nullptr);
if (Res.second)
- Res.first->second = &takeOwnership(std::make_unique<NegationValue>(Val));
+ Res.first->second = &create<NegationValue>(Val);
return *Res.first->second;
}
@@ -144,8 +140,7 @@
auto Res = ImplicationVals.try_emplace(std::make_pair(&LHS, &RHS), nullptr);
if (Res.second)
- Res.first->second =
- &takeOwnership(std::make_unique<ImplicationValue>(LHS, RHS));
+ Res.first->second = &create<ImplicationValue>(LHS, RHS);
return *Res.first->second;
}
@@ -157,8 +152,7 @@
auto Res = BiconditionalVals.try_emplace(makeCanonicalBoolValuePair(LHS, RHS),
nullptr);
if (Res.second)
- Res.first->second =
- &takeOwnership(std::make_unique<BiconditionalValue>(LHS, RHS));
+ Res.first->second = &create<BiconditionalValue>(LHS, RHS);
return *Res.first->second;
}
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -319,27 +319,59 @@
/// is assigned a storage location in the environment, otherwise returns null.
Value *getValue(const Expr &E, SkipPast SP) const;
+ /// Creates a `T` (some subclass of `StorageLocation`), forwarding `args` to
+ /// the constructor, and returns a reference to it.
+ ///
+ /// The analysis context takes ownership of the created object. The object
+ /// will be destroyed when the analysis context is destroyed.
+ template <typename T, typename... Args>
+ std::enable_if_t<std::is_base_of<StorageLocation, T>::value, T &>
+ create(Args &&...args) {
+ return DACtx->create<T>(std::forward<Args>(args)...);
+ }
+
+ /// Creates a `T` (some subclass of `Value`), forwarding `args` to the
+ /// constructor, and returns a reference to it.
+ ///
+ /// The analysis context takes ownership of the created object. The object
+ /// will be destroyed when the analysis context is destroyed.
+ template <typename T, typename... Args>
+ std::enable_if_t<std::is_base_of<Value, T>::value, T &>
+ create(Args &&...args) {
+ return DACtx->create<T>(std::forward<Args>(args)...);
+ }
+
/// Transfers ownership of `Loc` to the analysis context and returns a
/// reference to it.
///
+ /// This function is deprecated. Instead of
+ /// `takeOwnership(std::make_unique<SomeStorageLocation>(args))`, prefer
+ /// `create<SomeStorageLocation>(args)`.
+ ///
/// Requirements:
///
/// `Loc` must not be null.
template <typename T>
- std::enable_if_t<std::is_base_of<StorageLocation, T>::value, T &>
- takeOwnership(std::unique_ptr<T> Loc) {
+ LLVM_DEPRECATED("use create<T> instead", "")
+ std::enable_if_t<std::is_base_of<StorageLocation, T>::value,
+ T &> takeOwnership(std::unique_ptr<T> Loc) {
return DACtx->takeOwnership(std::move(Loc));
}
/// Transfers ownership of `Val` to the analysis context and returns a
/// reference to it.
///
+ /// This function is deprecated. Instead of
+ /// `takeOwnership(std::make_unique<SomeValue>(args))`, prefer
+ /// `create<SomeValue>(args)`.
+ ///
/// Requirements:
///
/// `Val` must not be null.
template <typename T>
- std::enable_if_t<std::is_base_of<Value, T>::value, T &>
- takeOwnership(std::unique_ptr<T> Val) {
+ LLVM_DEPRECATED("use create<T> instead", "")
+ std::enable_if_t<std::is_base_of<Value, T>::value, T &> takeOwnership(
+ std::unique_ptr<T> Val) {
return DACtx->takeOwnership(std::move(Val));
}
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -86,14 +86,51 @@
/*Logger=*/nullptr});
~DataflowAnalysisContext();
+ /// Creates a `T` (some subclass of `StorageLocation`), forwarding `args` to
+ /// the constructor, and returns a reference to it.
+ ///
+ /// The `DataflowAnalysisContext` takes ownership of the created object. The
+ /// object will be destroyed when the `DataflowAnalysisContext` is destroyed.
+ template <typename T, typename... Args>
+ std::enable_if_t<std::is_base_of<StorageLocation, T>::value, T &>
+ create(Args &&...args) {
+ // Note: If allocation of individual `StorageLocation`s turns out to be
+ // costly, consider creating specializations of `create<T>` for commonly
+ // used `StorageLocation` subclasses and make them use a `BumpPtrAllocator`.
+ return *cast<T>(
+ Locs.emplace_back(std::make_unique<T>(std::forward<Args>(args)...))
+ .get());
+ }
+
+ /// Creates a `T` (some subclass of `Value`), forwarding `args` to the
+ /// constructor, and returns a reference to it.
+ ///
+ /// The `DataflowAnalysisContext` takes ownership of the created object. The
+ /// object will be destroyed when the `DataflowAnalysisContext` is destroyed.
+ template <typename T, typename... Args>
+ std::enable_if_t<std::is_base_of<Value, T>::value, T &>
+ create(Args &&...args) {
+ // Note: If allocation of individual `Value`s turns out to be costly,
+ // consider creating specializations of `create<T>` for commonly used
+ // `Value` subclasses and make them use a `BumpPtrAllocator`.
+ return *cast<T>(
+ Vals.emplace_back(std::make_unique<T>(std::forward<Args>(args)...))
+ .get());
+ }
+
/// Takes ownership of `Loc` and returns a reference to it.
///
+ /// This function is deprecated. Instead of
+ /// `takeOwnership(std::make_unique<SomeStorageLocation>(args))`, prefer
+ /// `create<SomeStorageLocation>(args)`.
+ ///
/// Requirements:
///
/// `Loc` must not be null.
template <typename T>
- std::enable_if_t<std::is_base_of<StorageLocation, T>::value, T &>
- takeOwnership(std::unique_ptr<T> Loc) {
+ LLVM_DEPRECATED("use create<T> instead", "")
+ std::enable_if_t<std::is_base_of<StorageLocation, T>::value,
+ T &> takeOwnership(std::unique_ptr<T> Loc) {
assert(Loc != nullptr);
Locs.push_back(std::move(Loc));
return *cast<T>(Locs.back().get());
@@ -101,12 +138,17 @@
/// Takes ownership of `Val` and returns a reference to it.
///
+ /// This function is deprecated. Instead of
+ /// `takeOwnership(std::make_unique<SomeValue>(args))`, prefer
+ /// `create<SomeValue>(args)`.
+ ///
/// Requirements:
///
/// `Val` must not be null.
template <typename T>
- std::enable_if_t<std::is_base_of<Value, T>::value, T &>
- takeOwnership(std::unique_ptr<T> Val) {
+ LLVM_DEPRECATED("use create<T> instead", "")
+ std::enable_if_t<std::is_base_of<Value, T>::value, T &> takeOwnership(
+ std::unique_ptr<T> Val) {
assert(Val != nullptr);
Vals.push_back(std::move(Val));
return *cast<T>(Vals.back().get());
@@ -170,9 +212,9 @@
}
/// Creates an atomic boolean value.
- AtomicBoolValue &createAtomicBoolValue() {
- return takeOwnership(std::make_unique<AtomicBoolValue>());
- }
+ LLVM_DEPRECATED("use create<AtomicBoolValue> instead",
+ "create<AtomicBoolValue>")
+ AtomicBoolValue &createAtomicBoolValue() { return create<AtomicBoolValue>(); }
/// Creates a Top value for booleans. Each instance is unique and can be
/// assigned a distinct truth value during solving.
@@ -182,9 +224,8 @@
/// implementation so that `Top iff Top` has a consistent meaning, regardless
/// of the identity of `Top`. Moreover, I think the meaning should be
/// `false`.
- TopBoolValue &createTopBoolValue() {
- return takeOwnership(std::make_unique<TopBoolValue>());
- }
+ LLVM_DEPRECATED("use create<TopBoolValue> instead", "create<TopBoolValue>")
+ TopBoolValue &createTopBoolValue() { return create<TopBoolValue>(); }
/// Returns a boolean value that represents the conjunction of `LHS` and
/// `RHS`. Subsequent calls with the same arguments, regardless of their
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
