[PATCH] D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

It's also be nice if --config-file would also support being passed a directory. If a directory was passed it would append ".clang-tidy" to the path and load that file, WDYT?



================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:320-324
+    if (!Checks.empty()) {
+      llvm::errs() << "Error: --config-file and --checks are mutually "
+                      "exclusive. Specify only one.\n";
+      return nullptr;
+    }
----------------
I disagree with this check here, `Config` is not mutually exclusive with `Checks`, `Checks` gets applied atop of `Config`. So the same should happen when using `ConfigFile` with `Checks`


================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:337
+
+    Config.assign((*Text)->getBuffer());
+  }
----------------
Hiralo wrote:
> DmitryPolukhin wrote:
> > DmitryPolukhin wrote:
> > > I suggest creating new local variable with text of the config from `Config` or `ConfigFile` file content i.e. avoid modifying `Config` itself.
> > It doesn't compile and using local variable instead of changing command line option will make this code easier to read and understand:
> > ```
> > clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no matching member function for call to 'assign'
> >     Config.assign((*Text)->getBuffer());
> >     ~~~~~~~^~~~~~
> > ```
> > Could you please fix build and please upload diff with `arc diff` as @njames93 suggested so buildbot can test your changes?
> > I suggest creating new local variable with text of the config from `Config` or `ConfigFile` file content i.e. avoid modifying `Config` itself.
> 
> Sorry! probably I missed this comment!
> 
> std::string Temp = (*Text)->getBuffer();
> Config.assign(Temp); // ???
> 
> if so how code will enter into below block... because we want to reuse code-path of 'Config' correct?
> 
> if (!Config.empty()) {
>  if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig = parseConfiguration(Config)) {
> ...
> }
> 
> ?
Could extract the configuration loading into a lambda
```
  auto LoadConfig = [&](StringRef Configuration)
      -> std::unique_ptr<ClangTidyOptionsProvider> {
    llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
        parseConfiguration(Configuration);
    if (ParsedConfig)
      return std::make_unique<ConfigOptionsProvider>(
          GlobalOptions,
          ClangTidyOptions::getDefaults().mergeWith(DefaultOptions, 0),
          *ParsedConfig, OverrideOptions, std::move(FS));
    llvm::errs() << "Error: invalid configuration specified.\n"
                 << ParsedConfig.getError().message() << "\n";
    return nullptr;
  };
```

Then further down
```
if (!ConfigFile.empty()) {
  ...
  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
        llvm::MemoryBuffer::getFile(ConfigFile.c_str());
    if (std::error_code EC = Text.getError()) {
      llvm::errs() << "Can't read config-file '" << ConfigFile
                   << "': " << EC.message() << "\n";
      return nullptr;
    }

    return LoadConfig((*Text)->getBuffer());
}

if (!Config.empty())
  return LoadConfig(Config);
```

This has the added bonus of not having to needlessly copy the buffer


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

https://reviews.llvm.org/D89936