[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc0725865b188: [clang][dataflow] Generalize custom comparison
to return tri-value result. (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137334/new/
https://reviews.llvm.org/D137334
Files:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,27 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
const auto *Decl = Type->getAsCXXRecordDecl();
if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
Decl->getName() != "SpecialBool")
- return false;
+ return ComparisonResult::Unknown;
auto *IsSet1 = cast_or_null(Val1.getProperty("is_set"));
+auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet1 == nullptr)
- return true;
+ return IsSet2 == nullptr ? ComparisonResult::Same
+ : ComparisonResult::Different;
-auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet2 == nullptr)
- return false;
+ return ComparisonResult::Different;
return Env1.flowConditionImplies(*IsSet1) ==
- Env2.flowConditionImplies(*IsSet2);
+ Env2.flowConditionImplies(*IsSet2)
+ ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +512,19 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
// Nothing to say about a value that does not model an `OptionalInt`.
if (!Type->isRecordType() ||
Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
- return false;
+ return ComparisonResult::Unknown;
auto *Prop1 = Val1.getProperty("has_value");
auto *Prop2 = Val2.getProperty("has_value");
assert(Prop1 != nullptr && Prop2 != nullptr);
-return areEquivalentValues(*Prop1, *Prop2);
+return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
bool merge(QualType Type, const Value , const Environment ,
@@ -1182,12 +1186,12 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
// Changes to a sound approximation, which allows us to test whether we can
// (soundly) converge for some loops.
-return false;
+return ComparisonResult::Unknown;
}
};
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@
}
/// Returns true if and only if `Type` is an optional type.
-bool IsOptionalType(QualType Type) {
+bool isOptionalType(QualType Type) {
if (!Type->isRecordType())
return false;
// FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@ -222,7 +222,7 @@
/// For example, if `Type` is `optional>`, the result of this
/// function will be 2.
int countOptionalWrappers(const ASTContext , QualType Type) {
- if (!IsOptionalType(Type))
+ if (!isOptionalType(Type))
return 0;
return 1 + countOptionalWrappers(
ASTCtx,
@@ -720,12
[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
ymandel updated this revision to Diff 472978.
ymandel added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137334/new/
https://reviews.llvm.org/D137334
Files:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,27 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
const auto *Decl = Type->getAsCXXRecordDecl();
if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
Decl->getName() != "SpecialBool")
- return false;
+ return ComparisonResult::Unknown;
auto *IsSet1 = cast_or_null(Val1.getProperty("is_set"));
+auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet1 == nullptr)
- return true;
+ return IsSet2 == nullptr ? ComparisonResult::Same
+ : ComparisonResult::Different;
-auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet2 == nullptr)
- return false;
+ return ComparisonResult::Different;
return Env1.flowConditionImplies(*IsSet1) ==
- Env2.flowConditionImplies(*IsSet2);
+ Env2.flowConditionImplies(*IsSet2)
+ ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +512,19 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
// Nothing to say about a value that does not model an `OptionalInt`.
if (!Type->isRecordType() ||
Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
- return false;
+ return ComparisonResult::Unknown;
auto *Prop1 = Val1.getProperty("has_value");
auto *Prop2 = Val2.getProperty("has_value");
assert(Prop1 != nullptr && Prop2 != nullptr);
-return areEquivalentValues(*Prop1, *Prop2);
+return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
bool merge(QualType Type, const Value , const Environment ,
@@ -1182,12 +1186,12 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
// Changes to a sound approximation, which allows us to test whether we can
// (soundly) converge for some loops.
-return false;
+return ComparisonResult::Unknown;
}
};
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@
}
/// Returns true if and only if `Type` is an optional type.
-bool IsOptionalType(QualType Type) {
+bool isOptionalType(QualType Type) {
if (!Type->isRecordType())
return false;
// FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@ -222,7 +222,7 @@
/// For example, if `Type` is `optional>`, the result of this
/// function will be 2.
int countOptionalWrappers(const ASTContext , QualType Type) {
- if (!IsOptionalType(Type))
+ if (!isOptionalType(Type))
return 0;
return 1 + countOptionalWrappers(
ASTCtx,
@@ -720,12 +720,14 @@
TransferMatchSwitch(*Elt, getASTContext(), State);
}
-bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
-
[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.
Comment at:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:73-74
+/// Returns:
+/// `Same`:`Val1` is equivalent to `Val2`, according to the model.
+/// `Different`:`Val1` is distinct from `Val2`, according to the model.
+/// `Unknown`: The model does not determine a relationship between `Val1`
Comment at:
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1189-1191
+ ComparisonResult compare(QualType Type, const Value ,
const Environment , const Value ,
const Environment ) override {
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137334/new/
https://reviews.llvm.org/D137334
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
ymandel marked 2 inline comments as done. ymandel added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:365 if (IsSet1 == nullptr) - return true; + return IsSet2 ? ComparisonStatus::Same : ComparisonStatus::Different; sgatev wrote: > Why is it same if `IsSet1` is null and `IsSet2` isn't null? Fixed. that was a mistake. Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1185 - - bool compareEquivalent(QualType Type, const Value , - const Environment , const Value , sgatev wrote: > Why remove this? It's not the same as the default implementation, right? Correct -- that was a mistake (I think my first version had a sound default, which I later put back as unsound to keep this NFC). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137334/new/ https://reviews.llvm.org/D137334 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
ymandel updated this revision to Diff 472975.
ymandel marked 2 inline comments as done.
ymandel added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137334/new/
https://reviews.llvm.org/D137334
Files:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,27 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
const auto *Decl = Type->getAsCXXRecordDecl();
if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
Decl->getName() != "SpecialBool")
- return false;
+ return ComparisonResult::Unknown;
auto *IsSet1 = cast_or_null(Val1.getProperty("is_set"));
+auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet1 == nullptr)
- return true;
+ return IsSet2 == nullptr ? ComparisonResult::Same
+ : ComparisonResult::Different;
-auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet2 == nullptr)
- return false;
+ return ComparisonResult::Different;
return Env1.flowConditionImplies(*IsSet1) ==
- Env2.flowConditionImplies(*IsSet2);
+ Env2.flowConditionImplies(*IsSet2)
+ ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +512,19 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonResult compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
// Nothing to say about a value that does not model an `OptionalInt`.
if (!Type->isRecordType() ||
Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
- return false;
+ return ComparisonResult::Unknown;
auto *Prop1 = Val1.getProperty("has_value");
auto *Prop2 = Val2.getProperty("has_value");
assert(Prop1 != nullptr && Prop2 != nullptr);
-return areEquivalentValues(*Prop1, *Prop2);
+return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
bool merge(QualType Type, const Value , const Environment ,
@@ -1182,12 +1186,12 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
+ ComparisonResult compare(QualType Type, const Value ,
const Environment , const Value ,
const Environment ) override {
// Changes to a sound approximation, which allows us to test whether we can
// (soundly) converge for some loops.
-return false;
+return ComparisonResult::Unknown;
}
};
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@
}
/// Returns true if and only if `Type` is an optional type.
-bool IsOptionalType(QualType Type) {
+bool isOptionalType(QualType Type) {
if (!Type->isRecordType())
return false;
// FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@ -222,7 +222,7 @@
/// For example, if `Type` is `optional>`, the result of this
/// function will be 2.
int countOptionalWrappers(const ASTContext , QualType Type) {
- if (!IsOptionalType(Type))
+ if (!isOptionalType(Type))
return 0;
return 1 + countOptionalWrappers(
ASTCtx,
@@ -720,12 +720,14 @@
TransferMatchSwitch(*Elt, getASTContext(), State);
}
-bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
- const Value ,
-
[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
sgatev added inline comments.
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:52
+/// Indicates the result of a tentative comparison.
+enum class ComparisonStatus {
+ Same,
Alternative that I slightly prefer - `ComparisonResult`.
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:75
+/// `Different`:`Val1` is distinct from `Val2`, according to the model.
+/// `Unknown`: The model does not determine a relationship between `Val1`
+///and `Val2`.
Perhaps replace "does not" with "can't"?
Comment at:
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:365
if (IsSet1 == nullptr)
- return true;
+ return IsSet2 ? ComparisonStatus::Same : ComparisonStatus::Different;
Why is it same if `IsSet1` is null and `IsSet2` isn't null?
Comment at:
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1185
-
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
Why remove this? It's not the same as the default implementation, right?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137334/new/
https://reviews.llvm.org/D137334
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.
ymandel created this revision.
ymandel added reviewers: xazax.hun, sgatev, 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.
Currently, the API for a model's custom value comparison returns a
boolean. Therefore, models cannot distinguish between situations where the
values are recognized by the model and different and those where the values are
just not recognized. This patch changes the return value to a tri-valued enum,
allowing models to express "don't know".
This patch is essentially a NFC -- no practical differences result from this
change in this patch. But, it prepares for future patches (particularly,
upcoming patches for widening) which will take advantage of the new flexibility.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D137334
Files:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,26 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonStatus compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
const auto *Decl = Type->getAsCXXRecordDecl();
if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
Decl->getName() != "SpecialBool")
- return false;
+ return ComparisonStatus::Unknown;
auto *IsSet1 = cast_or_null(Val1.getProperty("is_set"));
+auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet1 == nullptr)
- return true;
+ return IsSet2 ? ComparisonStatus::Same : ComparisonStatus::Different;
-auto *IsSet2 = cast_or_null(Val2.getProperty("is_set"));
if (IsSet2 == nullptr)
- return false;
+ return ComparisonStatus::Different;
return Env1.flowConditionImplies(*IsSet1) ==
- Env2.flowConditionImplies(*IsSet2);
+ Env2.flowConditionImplies(*IsSet2)
+ ? ComparisonStatus::Same
+ : ComparisonStatus::Different;
}
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +511,19 @@
}
}
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
+ ComparisonStatus compare(QualType Type, const Value ,
+ const Environment , const Value ,
+ const Environment ) override {
// Nothing to say about a value that does not model an `OptionalInt`.
if (!Type->isRecordType() ||
Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
- return false;
+ return ComparisonStatus::Unknown;
auto *Prop1 = Val1.getProperty("has_value");
auto *Prop2 = Val2.getProperty("has_value");
assert(Prop1 != nullptr && Prop2 != nullptr);
-return areEquivalentValues(*Prop1, *Prop2);
+return areEquivalentValues(*Prop1, *Prop2) ? ComparisonStatus::Same
+ : ComparisonStatus::Different;
}
bool merge(QualType Type, const Value , const Environment ,
@@ -1181,14 +1184,6 @@
Env.setStorageLocation(*E, Loc);
}
}
-
- bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,
- const Environment ) override {
-// Changes to a sound approximation, which allows us to test whether we can
-// (soundly) converge for some loops.
-return false;
- }
};
class TopTest : public Test {
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@
}
/// Returns true if and only if `Type` is an optional type.
-bool IsOptionalType(QualType Type) {
+bool isOptionalType(QualType Type) {
if (!Type->isRecordType())
return false;
// FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@
