Quuxplusone added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:225 + ClangTidyOptions &&OverrideOptions, + ConfigFileHandlers &&ConfigHandlers); ---------------- njames93 wrote: > Quuxplusone wrote: > > I'd strongly recommend doing these two signatures in exactly the same way > > as you've done the others: pass by value and `std::move` out of it. > > Pass-by-rvalue-reference should be reserved for arcane stuff like > > hand-written move-constructors. You don't need pass-by-rvalue-reference > > here. > While I agree that rvalues in constructors is usually suspicious, in this > case it does make sense. > Firstly, FileOptionsBaseProviders constructors aren't exposed to the public > interface, so we don't really need to worry on that front. > Using r-values saves unneeded move constructor calls, which given that > ClangTidyGlobalOptions and ClangTidyOptions have non-trivial most > constructors, this is a slight win. > Is it still better to just pass by value though?? Well, IMHO it's better to just pass by value (for clarity). But it doesn't matter much either way. IMHO the "performance" angle doesn't matter because optimizing compilers, so basically at this point we're trading off "code clarity" versus "amount of bikeshedding in this PR," and I'm certainly willing to stop my bikeshedding, either way. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92267/new/ https://reviews.llvm.org/D92267 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
