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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits