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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
