[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thanks for the great work! This is mostly looking fine, but I've added a few comments. I could only make a quite superficial review so far, but I'll try to take a deeper look into this soon.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:70
+  static constexpr std::size_t WidthWithPadding = Width + (Width / 4);
+  char S[WidthWithPadding];
+  for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) {
----------------
I think, this function is not needed (see the comment below), but if it is, a single std::string and in-place std::reverse() would be better, imo.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:84-87
+/// Formats the bit representation of a numeric type as a string in groups of 4.
+template <typename T> static inline std::string formatBits(T &&V) {
+  return formatBitsImpl<sizeof(T) * CHAR_BIT>(V);
+}
----------------
I suppose that textual representation of the flags would be easier to understand in debug logs than just bits. Maybe single characters or other abbreviations, if this needs to be compact.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120
+#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
+
+  FB(None, 1),      //< Mix between the two parameters is not possible.
+  FB(Trivial, 2),   //< The two mix trivially, and are the exact same type.
+  FB(Canonical, 3), //< The two mix because the types refer to the same
+                    // CanonicalType, but we do not elaborate as to how.
+
----------------
I'd switch to scoped enums (`enum class`), remove the macro, and use simpler code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+  void sanitize() {
+    assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");
----------------
What should this method do and how it should be used? Same for Mix::sanitize()


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:295-297
+  if (llvm::any_of(
+          Check.IgnoredParameterNames,
+          [NodeName](const std::string &E) { return NodeName == E; })) {
----------------
Would `llvm::find(Check.IgnoredParameterNames, NodeName) != Check.IgnoredParameterNames.end()` work?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+
+  StringRef NodeTypeName = [Node]() {
+    const ASTContext &Ctx = Node->getASTContext();
----------------
No need for parentheses here.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:313-320
+    if (B.isMacroID()) {
+      LLVM_DEBUG(llvm::dbgs() << "\t\tBeginning is macro.\n");
+      B = SM.getTopMacroCallerLoc(B);
+    }
+    if (E.isMacroID()) {
+      LLVM_DEBUG(llvm::dbgs() << "\t\tEnding is macro.\n");
+      E = Lexer::getLocForEndOfToken(SM.getTopMacroCallerLoc(E), 0, SM, LO);
----------------
Is there a chance that you're trying to reimplement a part of Lexer::makeFileCharRange functionality here?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:129
+
+void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
+
----------------
Does the top-level const change anything with regard to the "mixability"? It's a purely implementation-side difference, callers could still swap parameters.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69560/new/

https://reviews.llvm.org/D69560