aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In general, I'm happy with this check (and the follow-ups), so I'm giving it my LG. However, it's complex enough that I think having some extra review from @alexfh, @hokein, or one of the other reviewers would be a good idea, so please hold off on landing it for a bit in case any of the other reviewers want to weigh in. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56 + +#ifndef NDEBUG + ---------------- 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.) 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
