[PATCH] D129547: [clang][dataflow] Generate readable form of boolean values for debugging purposes.

[Dmitri Gribenko via Phabricator via cfe-commits]

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