[clang] [Clang] Optimize -Wunsafe-buffer-usage. (PR #125492)

[Ilya Biryukov via cfe-commits]

================
@@ -294,63 +426,81 @@ isInUnspecifiedPointerContext(internal::Matcher<Stmt> InnerMatcher) {
   // 4. the operand of a pointer subtraction operation
   //    (i.e., computing the distance between two pointers); or ...
 
-  // clang-format off
-  auto CallArgMatcher = callExpr(
+  if (auto *CE = dyn_cast<CallExpr>(S)) {
+    if (const auto *FnDecl = CE->getDirectCallee();
+        FnDecl && FnDecl->hasAttr<UnsafeBufferUsageAttr>())
+      return;
     forEachArgumentWithParamType(
-      InnerMatcher,
-      isAnyPointer() /* array also decays to pointer type*/),
-    unless(callee(
-      functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
-
-  auto CastOperandMatcher =
-      castExpr(anyOf(hasCastKind(CastKind::CK_PointerToIntegral),
-		     hasCastKind(CastKind::CK_PointerToBoolean)),
-	       castSubExpr(allOf(hasPointerType(), InnerMatcher)));
-
-  auto CompOperandMatcher =
-      binaryOperator(hasAnyOperatorName("!=", "==", "<", "<=", ">", ">="),
-                     eachOf(hasLHS(allOf(hasPointerType(), InnerMatcher)),
-                            hasRHS(allOf(hasPointerType(), InnerMatcher))));
-
-  // A matcher that matches pointer subtractions:
-  auto PtrSubtractionMatcher =
-      binaryOperator(hasOperatorName("-"),
-		     // Note that here we need both LHS and RHS to be
-		     // pointer. Then the inner matcher can match any of
-		     // them:
-		     allOf(hasLHS(hasPointerType()),
-			   hasRHS(hasPointerType())),
-		     eachOf(hasLHS(InnerMatcher),
-			    hasRHS(InnerMatcher)));
-  // clang-format on
-
-  return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher,
-                    PtrSubtractionMatcher));
-  // FIXME: any more cases? (UPC excludes the RHS of an assignment.  For now we
-  // don't have to check that.)
-}
-
-// Returns a matcher that matches any expression 'e' such that `innerMatcher`
-// matches 'e' and 'e' is in an unspecified untyped context (i.e the expression
-// 'e' isn't evaluated to an RValue). For example, consider the following code:
+        *CE, [&InnerMatcher](QualType Type, const Expr *Arg) {
+          if (Type->isAnyPointerType())
+            InnerMatcher(Arg);
+        });
+  }
+
+  if (auto *CE = dyn_cast<CastExpr>(S)) {
+    if (CE->getCastKind() != CastKind::CK_PointerToIntegral &&
+        CE->getCastKind() != CastKind::CK_PointerToBoolean)
+      return;
+    if (!hasPointerType(*CE->getSubExpr()))
+      return;
+    InnerMatcher(CE->getSubExpr());
+  }
+
+  // Pointer comparison operator.
+  if (const auto *BO = dyn_cast<BinaryOperator>(S);
+      BO && (BO->getOpcode() == BO_EQ || BO->getOpcode() == BO_NE ||
+             BO->getOpcode() == BO_LT || BO->getOpcode() == BO_LE ||
+             BO->getOpcode() == BO_GT || BO->getOpcode() == BO_GE)) {
+    auto *LHS = BO->getLHS();
+    auto *RHS = BO->getRHS();
+    if (!hasPointerType(*LHS) || !hasPointerType(*RHS))
----------------
ilya-biryukov wrote:

I believe this code is not a direct translation compared to the original matcher.
The original matcher would be equivalent to:

```cpp
auto *LHS = BO->getLHS();
if (hasPointerType(LHS))
  InnerMatcher(LHS);

auto *RHS = BO->getRHS();
if (hasPointerType(RHS))
  InnerMatcher(RHS);
```

The caveat is that, I **think** in all the ASTs Clang produces both arguments of binary operator must either be pointers or non-pointers, so we end up with semantically equivalent code.

I would still advise to change this to match how matcher behaves exactly and ensure the translation is purely mechanical and easily verifiable.

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