[llvm-branch-commits] [clang] [SSAF][UnsafeBufferUsage] Remove UnsafeBufferUsageExtractor.h (PR #191931)

[Balázs Benics via llvm-branch-commits]

================
@@ -241,182 +273,195 @@ TEST_F(UnsafeBufferUsageTest, UnsafeBufferUsageSerializeTest) {
 //////////////////////////////////////////////////////////////
 
 TEST_F(UnsafeBufferUsageTest, SimpleFunctionWithUnsafePointer) {
-  auto Sum = setUpTest(R"cpp(
+  ASSERT_EQ(setUpTest(R"cpp(
     void foo(int *p) {
       p[5];
     }
-  )cpp",
-                       "foo");
+  )cpp"),
+            true);
+  const auto *Sum = getEntitySummary("foo");
 
   EXPECT_NE(Sum, nullptr);
   EXPECT_EQ(*Sum, makeSet(__LINE__, {{"p", 1U}}));
 }
 
 TEST_F(UnsafeBufferUsageTest, PointerArithmetic) {
-  auto Sum = setUpTest(R"cpp(
+  ASSERT_EQ(setUpTest(R"cpp(
     void foo(int *p, int *q) {
       *(p + 5);
       *(q - 3);
     }
-  )cpp",
-                       "foo");
+  )cpp"),
+            true);
+  const auto *Sum = getEntitySummary("foo");
 
   EXPECT_NE(Sum, nullptr);
   EXPECT_EQ(*Sum, makeSet(__LINE__, {{"p", 1U}, {"q", 1U}}));
 }
 
 TEST_F(UnsafeBufferUsageTest, PointerIncrementDecrement) {
-  auto Sum = setUpTest(R"cpp(
+  ASSERT_EQ(setUpTest(R"cpp(
     void foo(int *p, int *q, int *r, int *s) {
       (++p)[5];
       (q++)[5];
       (--r)[5];
       (s--)[5];
     }
-  )cpp",
-                       "foo");
+  )cpp"),
+            true);
+  const auto *Sum = getEntitySummary("foo");
 
   EXPECT_NE(Sum, nullptr);
   EXPECT_EQ(*Sum,
             makeSet(__LINE__, {{"p", 1U}, {"q", 1U}, {"r", 1U}, {"s", 1U}}));
 }
 
 TEST_F(UnsafeBufferUsageTest, PointerAssignment) {
-  auto Sum = setUpTest(R"cpp(
+  ASSERT_EQ(setUpTest(R"cpp(
     void foo(int *p, int *q) {
       (p = q + 5)[5];
     }
-  )cpp",
-                       "foo");
+  )cpp"),
+            true);
+  const auto *Sum = getEntitySummary("foo");
 
   EXPECT_NE(Sum, nullptr);
   EXPECT_EQ(*Sum, makeSet(__LINE__, {{"p", 1U}, {"q", 1U}}));
 }
 
 TEST_F(UnsafeBufferUsageTest, CompoundAssignment) {
-  auto Sum = setUpTest(R"cpp(
+  ASSERT_EQ(setUpTest(R"cpp(
     void foo(int *p, int *q) {
       (p += 5)[5];
       (q -= 3)[5];
     }
-  )cpp",
-                       "foo");
+  )cpp"),
+            true);
+  const auto *Sum = getEntitySummary("foo");
----------------
steakhal wrote:

I have a strong opinion about this.
My problem with `ASSERT_EQ()` is that to know what you are checking, you need to find the second operand of the assert. However, that can be arbitrarily pushed away from the open parenthesis even to a completely different line.
clang-format only makes this worse because it also indents it pretty far.

I had this problem (dislike) for checking if something is not `nullptr` using a `NE()` assertion.
Here it's just a lot more pronounced.

If we had been using `ASSERT_TRUE()`, however, this problem wouldn't even occur.
The alternative would be to swap the operands of `EQ` and have the constant at the front, but then people would still ask the question, why did they not use `ASSERT_TRUE()` instead if't pretty much what we are expressing here - leaving some unease feeling that the reader can't explain all aspects of the code.

IMO we should replace the checks of `setUpTest` to use `ASSERT_TRUE()`.

https://github.com/llvm/llvm-project/pull/191931