gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DebugSupport.h:24-25
+namespace dataflow {
+/// Utility functions which return a string representation for a boolean value
+/// `B`.
+///
----------------
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DebugSupport.h:35-36
+debugString(BoolValue &B,
+ llvm::DenseMap<AtomicBoolValue *, std::string> AtomNames);
+inline std::string debugString(BoolValue &B) { return debugString(B, {{}}); }
+
----------------
Does a default argument work?
================
Comment at: clang/lib/Analysis/FlowSensitive/DebugSupport.cpp:30-39
+ explicit DebugStringGenerator(
+ llvm::DenseMap<AtomicBoolValue *, std::string> AtomNames)
+ : Counter(0), AtomNames(AtomNames) {
+ llvm::StringSet<> Names;
+ for (auto &N : AtomNames) {
+ (void)N;
+ assert(Names.insert(N.second).second &&
----------------
To avoid even creating a set in the release mode.
================
Comment at: clang/lib/Analysis/FlowSensitive/DebugSupport.cpp:32
+ llvm::DenseMap<AtomicBoolValue *, std::string> AtomNames)
+ : Counter(0), AtomNames(AtomNames) {
+ llvm::StringSet<> Names;
----------------
================
Comment at: clang/lib/Analysis/FlowSensitive/DebugSupport.cpp:46
+ case Value::Kind::AtomicBool: {
+ S = formatv("({0})", getAtomName(&cast<AtomicBoolValue>(B)));
+ break;
----------------
================
Comment at: clang/unittests/Analysis/FlowSensitive/DebugSupportTest.cpp:21-32
+class BoolValueDebugStringTest : public ::testing::Test {
+protected:
+ test::BoolValueManager Bools;
+};
+
+TEST_F(BoolValueDebugStringTest, AtomicBoolean) {
+ // B0
----------------
It is usually better to avoid fixtures, to keep each test self-contained.
Please apply to the whole file.
================
Comment at: clang/unittests/Analysis/FlowSensitive/DebugSupportTest.cpp:30
+
+ auto Expected = R"((B0))";
+ EXPECT_THAT(debugString(*B), StrEq(Expected));
----------------
This should be just `B0` without parentheses.
================
Comment at: clang/unittests/Analysis/FlowSensitive/DebugSupportTest.cpp:39
+ auto Expected = R"((not
+ (B0)))";
+ EXPECT_THAT(debugString(*B), StrEq(Expected));
----------------
Here we should also have `B0` without parentheses. So `(not B0)`.
Please apply everywhere in the file.
================
Comment at: clang/unittests/Analysis/FlowSensitive/DebugSupportTest.cpp:134
+ // True
+ llvm::StringMap<AtomicBoolValue *> NamedBools;
+ auto True = cast<AtomicBoolValue>(Bools.atom());
----------------
`NamedBools` seems to be not used?..
================
Comment at: clang/unittests/Analysis/FlowSensitive/DebugSupportTest.cpp:171
+ // (False && B0) v (True v B1)
+ llvm::StringMap<AtomicBoolValue *> NamedBools;
+ auto True = cast<AtomicBoolValue>(Bools.atom());
----------------
`NamedBools` seems to be not used?..
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129547/new/
https://reviews.llvm.org/D129547
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
