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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58
+
+namespace {
+
----------------
Is there a need for the anonymous namespace? (We typically only use them when defining a class and try to make them as narrow in scope as possible, and use `static` for function declarations instead.)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:67
+/// The language features involved in allowing the mix between two parameters.
+enum MixFlags : unsigned char {
+  MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!
----------------
Does using the fixed underlying type buy us something? As best I can tell, this will introduce a fair amount of implicit promotions to `int` anyway.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:90-91
+
+  void sanitise() {
+    assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec");
+    // TODO: There will be statements here in further extensions of the check.
----------------
Heh, I don't know if we have any guidance on US English vs British English spellings. ;-)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:105
+
+  void sanitise() { Data.sanitise(); }
+  MixFlags flags() const { return Data.Flags; }
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:176-177
+                                       const FunctionDecl *FD,
+                                       const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");
----------------
Outside of field declarations, we typically only `const`-qualify pointer/reference types.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+                                       const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");
----------------
Some interesting test cases to consider: varargs functions and K&R C functions


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:196
+    // parameters that can be messed up at a call site.
+    decltype(Ret.Mixes) MixesOfIth;
+    for (std::size_t J = StartIndex; J < I; ++J) {
----------------
Preferable to spell this type out to make it easier on someone reading the code to understand the type.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+      Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};
----------------
Hmm, one downside to using the printing policy to get the node name is that there can be alternative spellings for the same type that the user might reasonably expect to be applied. e.g., if the user expects to ignore `bool` datatypes and the printing policy wants to spell the type as `_Bool`, this won't behave as expected.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:284
+/// The default value for the MinimumLength check option.
+static constexpr unsigned DefaultMinimumLength = 2;
+
----------------
It might be good to move this to a more prominent location since it's a default value.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:301
+                         "Iterator", "inputit", "InputIt", "forwardit",
+                         "FowardIt", "bidirit", "BidirIt", "constiterator",
+                         "const_iterator", "Const_Iterator", "Constiterator",
----------------
If we're going to include `ForwardIt`, we probably want things like `RandomIt` as well?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+                         "FowardIt", "bidirit", "BidirIt", "constiterator",
+                         "const_iterator", "Const_Iterator", "Constiterator",
+                         "ConstIterator"});
----------------
`reverse_iterator` and `reverse_const_iterator` too?

How about ranges?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+    // Unfortunately, undersquiggly highlights are for some reason chewed up
+    // and not respected by diagnostics from Clang-Tidy. :(
+    diag(First->getLocation(), "the first parameter in the range is '%0'",
----------------
Can you post a screenshot of what you mean?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:30
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+  /// The minimum length of an adjacent swappable parameter range required for
----------------
Do these data members need to be public?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this function.
+
----------------
I think this is a case where we could warn when the declaration is outside of a system header (perhaps as an option).

Thinking about it a bit more, declarations and definitions provide a novel way to get another kind of swap:
```
void func(int x, int y);
void func(int y, int x) {} // Oops, the swap was probably not intentional
```
which may or may not be interesting for a check (or its heuristics).


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:14
+
+S *allocate() { return nullptr; }                           // NO-WARN: 1 parameter.
+void allocate(S **Out) {}                                   // NO-WARN: 1 parameter.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560