whisperity added a comment. 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.) ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56 + +#ifndef NDEBUG + ---------------- 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.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69560/new/ https://reviews.llvm.org/D69560 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
