[clang-tools-extra] [clang-tidy] Add bugprone-chained-comparison check (PR #76365)

[Julian Schmidt via cfe-commits]

================
@@ -0,0 +1,147 @@
+//===--- ChainedComparisonCheck.cpp - clang-tidy --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ChainedComparisonCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include <algorithm>
+#include <array>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+static bool isExprAComparisonOperator(const Expr *E) {
+  if (const auto *Op = dyn_cast_or_null<BinaryOperator>(E->IgnoreImplicit()))
+    return Op->isComparisonOp();
+  if (const auto *Op =
+          dyn_cast_or_null<CXXOperatorCallExpr>(E->IgnoreImplicit()))
+    return Op->isComparisonOp();
+  return false;
+}
+
+namespace {
+AST_MATCHER(BinaryOperator,
+            hasBinaryOperatorAChildComparisonOperatorWithoutParen) {
+  return isExprAComparisonOperator(Node.getLHS()) ||
+         isExprAComparisonOperator(Node.getRHS());
+}
+
+AST_MATCHER(CXXOperatorCallExpr,
+            hasCppOperatorAChildComparisonOperatorWithoutParen) {
+  return std::any_of(Node.arg_begin(), Node.arg_end(),
+                     isExprAComparisonOperator);
+}
+
+struct ChainedComparisonData {
+  llvm::SmallString<256U> Name;
+  llvm::SmallVector<const Expr *, 32U> Operands;
+
+  explicit ChainedComparisonData(const Expr *Op) { extract(Op); }
+
+private:
+  void add(const Expr *Operand);
+  void add(llvm::StringRef Opcode);
+  void extract(const Expr *Op);
+  bool extract(const BinaryOperator *Op);
+  bool extract(const CXXOperatorCallExpr *Op);
+};
+
+void ChainedComparisonData::add(const Expr *Operand) {
+  if (!Name.empty())
+    Name += ' ';
+  Name += 'v';
+  Name += std::to_string(Operands.size());
+  Operands.push_back(Operand);
+}
+
+void ChainedComparisonData::add(llvm::StringRef Opcode) {
+  Name += ' ';
+  Name += Opcode.str();
+}
+
+bool ChainedComparisonData::extract(const BinaryOperator *Op) {
+  if (!Op)
+    return false;
+
+  if (isExprAComparisonOperator(Op->getLHS()))
+    extract(Op->getLHS()->IgnoreImplicit());
+  else
+    add(Op->getLHS()->IgnoreUnlessSpelledInSource());
----------------
5chmidti wrote:

The `IgnoreUnlessSpelledInSource` is not needed (same for the other `add` calls). Beyond using the `Expr` passed to `add` to get source locations, nothing else is being done with it. 
When combined with my suggestion about streaming in the whole source range, calling `IgnoreUnlessSpelledInSource` will result in parentheses not being present in the source range, e.g. for `bool result = (x < y)<(z && t)>(a == b);`, the notes would say the range of `v0` is `x < y`, not `(x < y)`. (the parentheses are admittedly a small difference)

A suggestion I'm indifferent about: save the source ranges of operands, instead of the `Expr`s.

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