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

[Hiral via Phabricator via cfe-commits]

Hiralo marked 6 inline comments as done.
Hiralo added inline comments.


================
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;
+    }
----------------
DmitryPolukhin wrote:
> Hiralo wrote:
> > DmitryPolukhin wrote:
> > > DmitryPolukhin wrote:
> > > > njames93 wrote:
> > > > > 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`
> > > > +1
> > > Clarify: +1 to @njames93 that we don't need this check.
> > > 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`
> > 
> > ok, will remove these two checks :)
> I think we need to keep the first check. There is no reason to specify both `--config` and `--config-file` or give on of them precedence.
> I think we need to keep the first check. There is no reason to specify both `--config` and `--config-file` or give on of them precedence.

Kept '--config-file' and '--config' mutually exclusive.




================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:337
+
+    Config.assign((*Text)->getBuffer());
+  }
----------------
njames93 wrote:
> Hiralo wrote:
> > 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)) {
> > > ...
> > > }
> > > 
> > > ?
> > > 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?
> > 
> > Let me try.
> 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
> > 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?
> 
> Let me try.

(Finally could make setup build on top of master and make arc work...)

Please review updated patch. with 'arc diff' :)


================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:337
+
+    Config.assign((*Text)->getBuffer());
+  }
----------------
Hiralo wrote:
> njames93 wrote:
> > Hiralo wrote:
> > > 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)) {
> > > > ...
> > > > }
> > > > 
> > > > ?
> > > > 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?
> > > 
> > > Let me try.
> > 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
> > > 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?
> > 
> > Let me try.
> 
> (Finally could make setup build on top of master and make arc work...)
> 
> Please review updated patch. with 'arc diff' :)
> Could extract the configuration loading into a lambda
> This has the added bonus of not having to needlessly copy the buffer

Thanks njames93 for lambda fun :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936