[PATCH] D92267: [clang-tidy][NFC] Use moves instead of copies when constructing OptionsProviders.

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:176
 public:
-  DefaultOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
-                         const ClangTidyOptions &Options)
-      : GlobalOptions(GlobalOptions), DefaultOptions(Options) {}
+  DefaultOptionsProvider(ClangTidyGlobalOptions GlobalOptions,
+                         ClangTidyOptions Options)
----------------
Quuxplusone wrote:
> If you're refactoring ctors anyway, perhaps add `explicit` to all of them? (The only reason to have a non-`explicit` ctor is if you want to enable callers to write `DefaultOptionsProvider d = {gopt, opt};` instead of `auto d = DefaultOptionsProvider(gopt, opt);`.) But this would be scope creep, of course.
`explicit`/`non-explicit` doesn't really affect much here. These constructors are only used for constructing base classes or forwarding args to `std::make_unique`.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:225
+                          ClangTidyOptions &&OverrideOptions,
+                          ConfigFileHandlers &&ConfigHandlers);
 
----------------
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??


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92267