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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69560#2629616 <https://reviews.llvm.org/D69560#2629616>, @whisperity wrote:

> In D69560#2629555 <https://reviews.llvm.org/D69560#2629555>, @aaron.ballman wrote:
>
>> [...] so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.
>
> Due to how the patch itself only being useful in practice if all the pieces fall into place, I will definitely keep circling about until the **entire** patch tree is ✔. (Including the filtering heuristics, etc.)

Good idea. :-)



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+
----------------
whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to remove the debugging code now that the check is approaching its final form?
> > > Actually, I would prefer not to. I removed every debug thing possible. However, and this is speaking from experience because I wrote this check two times already from basically scratch... the rest of the debug code that is part of the patch has to be there. If anything goes nuts, especially if there would be false positives later... it would be impossible to figure out what is going on during the modelling without seeing all the steps being taken.
> > We don't often leave debugging statements in because they tend to cause a maintenance burden that doesn't justify their benefit. However, I agree with your observation that this code is quite difficult to reason through without debugging aids.
> > 
> > I don't insist on removing the code yet, but would say we should remain open to the idea if it causes a burden in practice. (Either in terms of keeping the debugging code up to date as the check evolves, but also in terms of compile time performance for the check if the debugging code paths turn out to be expensive on build times.)
> All debugging code is wrapped into the `LLVM_DEBUG` macro specifically (that's why I even put this little "print bits" function behind `NDEBUG`) so they are not part of the builds.
> 
> I think in general //if// someone puts the effort into reading the code and has an interactive debugger they could figure it out (especially if they keep track of the recursion constantly), I tried to make everything nicely padded and all the variable names and control flow to make sense. (Of course the wealth of complexity comes in the follow-up patches.)
> All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's why I even put this little "print bits" function behind NDEBUG) so they are not part of the builds.

They're part of debug builds still (which is the build configuration I tend to use most often). That said, I think it's fine to leave the debugging code in for now, especially as the check is being actively worked on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560