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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


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


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

I would like to say that having an `f(RangeTy, RangeTy)` is //exactly// as problematic (especially if both are non-const) as having `f(int, int)` or `f(void*, void*)`, and should be taken care of the same way (relatedness heuristic, name heuristic).
The idea behind ignoring iterator-ly named things was that //"Iterators more often than not comes in pairs and you can't(*) do anything about it"//.

(*): Use ranges. Similarly how `draw(int x, int y)` is `draw(Point2D)` if we are enhancing type safety.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560