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

[• Quuxplusone]

clang-tools-extra/clang-tidy/ClangTidyOptions.h

176–179

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.

225

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.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.h

176–179

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.

225

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??

[• Quuxplusone]

clang-tools-extra/clang-tidy/ClangTidyOptions.h

225

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. :)

[njames93]

Use pass by value for FileOptionsBaseProvider constructor.
Ping?

[aaron.ballman]

LGTM

clang-tools-extra/clang-tidy/ClangTidyOptions.h

public:
   /// specified \p FileName.
   ClangTidyOptions getOptions(llvm::StringRef FileName);
 };
 
 /// Implementation of the \c ClangTidyOptionsProvider interface, which
 /// returns the same options for all files.
 class DefaultOptionsProvider : public ClangTidyOptionsProvider {
 public:
-  DefaultOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
-                         const ClangTidyOptions &Options)
-      : GlobalOptions(GlobalOptions), DefaultOptions(Options) {}
+  DefaultOptionsProvider(ClangTidyGlobalOptions GlobalOptions,
+                         ClangTidyOptions Options)
+      : GlobalOptions(std::move(GlobalOptions)),
+        DefaultOptions(std::move(Options)) {}
   const ClangTidyGlobalOptions &getGlobalOptions() override {
     return GlobalOptions;
   }
   std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
 
 private:
   ClangTidyGlobalOptions GlobalOptions;
   ClangTidyOptions DefaultOptions;
 };
 
 class FileOptionsBaseProvider : public DefaultOptionsProvider {
-public:
+protected:
   // A pair of configuration file base name and a function parsing
   // configuration from text in the corresponding format.
   typedef std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions>(
                                      llvm::StringRef)>>
       ConfigFileHandler;
 
   /// Configuration file handlers listed in the order of priority.
   ///
protected:
   /// return std::make_unique<FileOptionsProvider>(
   ///     GlobalOptions, DefaultOptions, OverrideOptions, ConfigHandlers);
   /// \endcode
   ///
   /// With the order of handlers shown above, the ".my-tidy-config" file would
   /// take precedence over ".clang-tidy" if both reside in the same directory.
   typedef std::vector<ConfigFileHandler> ConfigFileHandlers;
 
-  FileOptionsBaseProvider(
-      const ClangTidyGlobalOptions &GlobalOptions,
-      const ClangTidyOptions &DefaultOptions,
-      const ClangTidyOptions &OverrideOptions,
-      llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
-
-  FileOptionsBaseProvider(const ClangTidyGlobalOptions &GlobalOptions,
-                          const ClangTidyOptions &DefaultOptions,
-                          const ClangTidyOptions &OverrideOptions,
-                          const ConfigFileHandlers &ConfigHandlers);
+  FileOptionsBaseProvider(ClangTidyGlobalOptions GlobalOptions,
+                          ClangTidyOptions DefaultOptions,
+                          ClangTidyOptions OverrideOptions,
+                          llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
+
+  FileOptionsBaseProvider(ClangTidyGlobalOptions GlobalOptions,
+                          ClangTidyOptions DefaultOptions,
+                          ClangTidyOptions OverrideOptions,
+                          ConfigFileHandlers ConfigHandlers);
 
 protected:
   void addRawFileOptions(llvm::StringRef AbsolutePath,
                          std::vector<OptionsSource> &CurOptions);
 
   /// Try to read configuration files from \p Directory using registered
   /// \c ConfigHandlers.
   llvm::Optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
 
   llvm::StringMap<OptionsSource> CachedOptions;
   ClangTidyOptions OverrideOptions;
   ConfigFileHandlers ConfigHandlers;
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
 };
 
 /// Implementation of ClangTidyOptions interface, which is used for
 /// '-config' command-line option.
 class ConfigOptionsProvider : public FileOptionsBaseProvider {
 public:
   ConfigOptionsProvider(
-      const ClangTidyGlobalOptions &GlobalOptions,
-      const ClangTidyOptions &DefaultOptions,
-      const ClangTidyOptions &ConfigOptions,
-      const ClangTidyOptions &OverrideOptions,
+      ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+      ClangTidyOptions ConfigOptions, ClangTidyOptions OverrideOptions,
       llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
   std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
 
 private:
   ClangTidyOptions ConfigOptions;
 };
 
 /// Implementation of the \c ClangTidyOptionsProvider interface, which
public:
   /// \c getGlobalOptions.
   ///
   /// \param DefaultOptions are used for all settings not specified in a
   /// configuration file.
   ///
   /// If any of the \param OverrideOptions fields are set, they will override
   /// whatever options are read from the configuration file.
   FileOptionsProvider(
-      const ClangTidyGlobalOptions &GlobalOptions,
-      const ClangTidyOptions &DefaultOptions,
-      const ClangTidyOptions &OverrideOptions,
+      ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+      ClangTidyOptions OverrideOptions,
       llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
 
   /// Initializes the \c FileOptionsProvider instance with a custom set
   /// of configuration file handlers.
   ///
   /// \param GlobalOptions are just stored and returned to the caller of
   /// \c getGlobalOptions.
   ///
   /// \param DefaultOptions are used for all settings not specified in a
   /// configuration file.
   ///
   /// If any of the \param OverrideOptions fields are set, they will override
   /// whatever options are read from the configuration file.
   ///
   /// \param ConfigHandlers specifies a custom set of configuration file
   /// handlers. Each handler is a pair of configuration file name and a function
   /// that can parse configuration from this file type. The configuration files
   /// in each directory are searched for in the order of appearance in
   /// \p ConfigHandlers.
-  FileOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
-                      const ClangTidyOptions &DefaultOptions,
-                      const ClangTidyOptions &OverrideOptions,
-                      const ConfigFileHandlers &ConfigHandlers);
+  FileOptionsProvider(ClangTidyGlobalOptions GlobalOptions,
+                      ClangTidyOptions DefaultOptions,
+                      ClangTidyOptions OverrideOptions,
+                      ConfigFileHandlers ConfigHandlers);
 
   std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
 };
 
 /// Parses LineFilter from JSON and stores it to the \p Options.
 std::error_code parseLineFilter(llvm::StringRef LineFilter,
                                 ClangTidyGlobalOptions &Options);
 

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

 std::vector<OptionsSource>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector<OptionsSource> Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
   return Result;
 }
 
 ConfigOptionsProvider::ConfigOptionsProvider(
-    const ClangTidyGlobalOptions &GlobalOptions,
-    const ClangTidyOptions &DefaultOptions,
-    const ClangTidyOptions &ConfigOptions,
-    const ClangTidyOptions &OverrideOptions,
+    ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+    ClangTidyOptions ConfigOptions, ClangTidyOptions OverrideOptions,
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
-    : FileOptionsBaseProvider(GlobalOptions, DefaultOptions, OverrideOptions,
-                              FS),
-      ConfigOptions(ConfigOptions) {}
+    : FileOptionsBaseProvider(std::move(GlobalOptions),
+                              std::move(DefaultOptions),
+                              std::move(OverrideOptions), std::move(FS)),
+      ConfigOptions(std::move(ConfigOptions)) {}
 
 std::vector<OptionsSource>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector<OptionsSource> RawOptions =
       DefaultOptionsProvider::getRawOptions(FileName);
   if (ConfigOptions.InheritParentConfig.getValueOr(false)) {
     LLVM_DEBUG(llvm::dbgs()
                << "Getting options for file " << FileName << "...\n");
     assert(FS && "FS must be set.");
 
     llvm::SmallString<128> AbsoluteFilePath(FileName);
 
     if (!FS->makeAbsolute(AbsoluteFilePath)) {
       addRawFileOptions(AbsoluteFilePath, RawOptions);
     }
   }
   RawOptions.emplace_back(ConfigOptions,
                           OptionsSourceTypeConfigCommandLineOption);
   RawOptions.emplace_back(OverrideOptions,
                           OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }
 
 FileOptionsBaseProvider::FileOptionsBaseProvider(
-    const ClangTidyGlobalOptions &GlobalOptions,
-    const ClangTidyOptions &DefaultOptions,
-    const ClangTidyOptions &OverrideOptions,
+    ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+    ClangTidyOptions OverrideOptions,
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS)
-    : DefaultOptionsProvider(GlobalOptions, DefaultOptions),
-      OverrideOptions(OverrideOptions), FS(std::move(VFS)) {
+    : DefaultOptionsProvider(std::move(GlobalOptions),
+                             std::move(DefaultOptions)),
+      OverrideOptions(std::move(OverrideOptions)), FS(std::move(VFS)) {
   if (!FS)
     FS = llvm::vfs::getRealFileSystem();
   ConfigHandlers.emplace_back(".clang-tidy", parseConfiguration);
 }
 
 FileOptionsBaseProvider::FileOptionsBaseProvider(
-    const ClangTidyGlobalOptions &GlobalOptions,
-    const ClangTidyOptions &DefaultOptions,
-    const ClangTidyOptions &OverrideOptions,
-    const FileOptionsBaseProvider::ConfigFileHandlers &ConfigHandlers)
-    : DefaultOptionsProvider(GlobalOptions, DefaultOptions),
-      OverrideOptions(OverrideOptions), ConfigHandlers(ConfigHandlers) {}
+    ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+    ClangTidyOptions OverrideOptions,
+    FileOptionsBaseProvider::ConfigFileHandlers ConfigHandlers)
+    : DefaultOptionsProvider(std::move(GlobalOptions),
+                             std::move(DefaultOptions)),
+      OverrideOptions(std::move(OverrideOptions)),
+      ConfigHandlers(std::move(ConfigHandlers)) {}
 
 void FileOptionsBaseProvider::addRawFileOptions(
     llvm::StringRef AbsolutePath, std::vector<OptionsSource> &CurOptions) {
   auto CurSize = CurOptions.size();
 
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef Path = llvm::sys::path::parent_path(AbsolutePath);
for (StringRef CurrentPath = Path; !CurrentPath.empty();
     }
   }
   // Reverse order of file configs because closer configs should have higher
   // priority.
   std::reverse(CurOptions.begin() + CurSize, CurOptions.end());
 }
 
 FileOptionsProvider::FileOptionsProvider(
-    const ClangTidyGlobalOptions &GlobalOptions,
-    const ClangTidyOptions &DefaultOptions,
-    const ClangTidyOptions &OverrideOptions,
+    ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+    ClangTidyOptions OverrideOptions,
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS)
-    : FileOptionsBaseProvider(GlobalOptions, DefaultOptions, OverrideOptions,
-                              VFS){}
+    : FileOptionsBaseProvider(std::move(GlobalOptions),
+                              std::move(DefaultOptions),
+                              std::move(OverrideOptions), std::move(VFS)) {}
 
 FileOptionsProvider::FileOptionsProvider(
-    const ClangTidyGlobalOptions &GlobalOptions,
-    const ClangTidyOptions &DefaultOptions,
-    const ClangTidyOptions &OverrideOptions,
-    const FileOptionsBaseProvider::ConfigFileHandlers &ConfigHandlers)
-    : FileOptionsBaseProvider(GlobalOptions, DefaultOptions, OverrideOptions,
-                              ConfigHandlers) {}
+    ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
+    ClangTidyOptions OverrideOptions,
+    FileOptionsBaseProvider::ConfigFileHandlers ConfigHandlers)
+    : FileOptionsBaseProvider(
+          std::move(GlobalOptions), std::move(DefaultOptions),
+          std::move(OverrideOptions), std::move(ConfigHandlers)) {}
 
 // FIXME: This method has some common logic with clang::format::getStyle().
 // Consider pulling out common bits to a findParentFileWithName function or
 // similar.
 std::vector<OptionsSource>
 FileOptionsProvider::getRawOptions(StringRef FileName) {
   LLVM_DEBUG(llvm::dbgs() << "Getting options for file " << FileName
                           << "...\n");

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

if (UseColor.getNumOccurrences() > 0)
     OverrideOptions.UseColor = UseColor;
 
   auto LoadConfig = [&](StringRef Configuration)
       -> std::unique_ptr<ClangTidyOptionsProvider> {
     llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
         parseConfiguration(Configuration);
     if (ParsedConfig)
       return std::make_unique<ConfigOptionsProvider>(
-          GlobalOptions,
+          std::move(GlobalOptions),
           ClangTidyOptions::getDefaults().merge(DefaultOptions, 0),
-          *ParsedConfig, OverrideOptions, std::move(FS));
+          std::move(*ParsedConfig), std::move(OverrideOptions), std::move(FS));
     llvm::errs() << "Error: invalid configuration specified.\n"
                  << ParsedConfig.getError().message() << "\n";
     return nullptr;
   };
 
   if (ConfigFile.getNumOccurrences() > 0) {
     if (Config.getNumOccurrences() > 0) {
       llvm::errs() << "Error: --config-file and --config are "
if (ConfigFile.getNumOccurrences() > 0) {
     }
 
     return LoadConfig((*Text)->getBuffer());
   }
 
   if (Config.getNumOccurrences() > 0)
     return LoadConfig(Config);
 
-  return std::make_unique<FileOptionsProvider>(GlobalOptions, DefaultOptions,
-                                                OverrideOptions, std::move(FS));
+  return std::make_unique<FileOptionsProvider>(
+      std::move(GlobalOptions), std::move(DefaultOptions),
+      std::move(OverrideOptions), std::move(FS));
 }
 
 llvm::IntrusiveRefCntPtr<vfs::FileSystem>
 getVfsFromFile(const std::string &OverlayFile,
                llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS) {
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
       BaseFS->getBufferForFile(OverlayFile);
   if (!Buffer) {