This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Hiralo on Oct 22 2020, 12:13 AM.

Details

Summary

commit 653cb7912bbe4daabc8d6dd6dca71b2cf9a10365 (HEAD -> master)
Author: Hiral Oza <hiral.oza@netapp.com>
Date: Fri Oct 30 05:07:19 2020 -0400

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

Let clang-tidy to read config from specified file.

This option internally works exactly the same way as --config option
after reading specified config file.

This option works as helper for --config in following few cases:
 - when there is shell limitation to read file content to the command line,
 - when there is limitation to command line length,
 - setting right build dependencies, and
 - others

--config-file and --config options are made mutually exclusive.

Example usage:
 $ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
 ...this will read config from /some/path/myTidyConfigFile and
    avoid searching '.clang-tidy' in parent-dirs.

May speed-up tidy runtime since now it will just look-up <file-path>
instead of searching ".clang-tidy" in parent-dirs.

Reviewed By: DmitryPolukhin, njames93

Differential Revision: https://reviews.llvm.org/D89936

Diff Detail

Event Timeline

Hiralo created this revision.Oct 22 2020, 12:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 12:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Hiralo requested review of this revision.Oct 22 2020, 12:13 AM

I'm not sure that we need additional option to read configuration from file but, if we do need, I think this diff needs some improvements + test for new option.

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

Is it actually required to check absolute path, link it or not, etc.? Why not just try reading file with provided filename and report error if it fails?

408

It seems like some debug prints.

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

I'm not sure that we need it here. I would reuse code path for --config options as much as possible and implement new option as a simple wrapper that reads content of the file and interpret it as --config option. Moreover I think it should not be possible to specify both options in command line.

233

It looks like the second argument was added only for overload resolution. But I think it is better to rename this function. After all it is not "try" anymore because it reports fatal error in case of errors.

DmitryPolukhin added inline comments.Oct 26 2020, 3:15 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
76

I would call it something like --config-file

Thanks for your kind review.
Working on it...

Hiralo marked 3 inline comments as done.Oct 26 2020, 10:32 PM
Hiralo marked an inline comment as done.Oct 26 2020, 11:30 PM
Hiralo added inline comments.
clang-tools-extra/clang-tidy/ClangTidyOptions.h
68

it should not be possible to specify both options in command line.

Made local changes with...

$ ./bin/clang-tidy -config="{Checks: '*'}" --config-file=/some/path/config --
Error: --config-file and --config are mutually exclusive. Specify only one.

$ ./bin/clang-tidy --checks='*' --config-file=/some/path/config --
Error: --config-file and --checks are mutually exclusive. Specify only one.

233

Renamed function to 'readConfigFile'.

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

renamed

Hiralo marked an inline comment as done.Oct 26 2020, 11:39 PM
Hiralo marked an inline comment as done.Oct 26 2020, 11:42 PM
Hiralo added inline comments.
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
324

Tried it and seems we don't need this changes since down the line below code emits required error message...

if (std::error_code EC = Text.getError()) {
std::string Msg;
llvm::raw_string_ostream ErrStream(Msg);
ErrStream << " Can't read <" << Path << ">: " << EC.message() << "\n";
llvm::report_fatal_error(ErrStream.str());
}

Hiralo updated this revision to Diff 300911.Oct 27 2020, 12:58 AM
Hiralo marked an inline comment as done.

Incorporated review comments and updated patch.
(a) renamed var 'ClangTidyConfig' with 'ConfigFile'
(b) renamed function 'tryReadConfigFile(AbsoluteFilePath, true);' to 'readConfigFile(ConfigPath)'
(c) renamed command-line option to '--config-file'
(d) made --config-file and --checks mutually exclusive
(e) make --config-file and --config mutually exclusive
(f) removed extra checks -- making absolute-path, isFile and isLink checks

...I think this diff needs some improvements

Thanks @DmitryPolukhin for your valuable comments/feedback.
Please review updated patch.

+ test for new option.

I am planning to add following test-case...
<diff>
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/read_file_config.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/read_file_config.cpp
.......
RUN: clang-tidy -checks="-*,clang-analyzer-*" %T/read-file-config/test.cpp | grep "warning: .*\[clang-analyzer-deadcode.DeadStores\]$"
+
RUN: clang-tidy -config-file=%T/read-file-config/.clang-tidy %T/read-file-config/test.cpp | grep "warning: .*\[clang-analyzer-deadcode.DeadStores\]$"
</diff>

njames93 retitled this revision from clang-tidy: adding "--clang-tidy-config=<file-path>" to specify custom config file to [clang-tidy] adding "--clang-tidy-config=<file-path>" to specify custom config file.Oct 27 2020, 2:19 AM

@Hiralo, it looks like you uploaded wrong diff because your previous revision is shown as "base" version. Base revision should be clang-tidy sources without your changes.

Please add Lit test for the new option, see https://llvm.org/docs/TestingGuide.html for more details and existing clang-tidy tests.

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

I think you can make this option much simpler if you just read file content and use it or Config here. No changes in clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.

Hiralo added inline comments.Oct 27 2020, 5:08 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
316

Sorry, didn't get. Can you please elaborate/pseudocode?

Hiralo added inline comments.Oct 27 2020, 5:16 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
316

I think you can make this option much simpler if you just read file content and use it or Config here. No changes in clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.

Are you suggesting to...
(a) as per this patch have option '--config-file' (ClangTidyMain.cpp changes)
(b) read this file in ClangTidyMain.cpp itself and
(c) pass YAML o/p to code-flow-of-config option (again change only in ClangTidyMain.cpp).

if so...

Shouldn't at call to tryReadConfigFile() kind of call in ClangTidyMain.cpp?

Can you please point me to LLVM/Clang File IO calls/examples?

Hiralo added inline comments.Oct 27 2020, 5:29 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
316

From code point of view... within ClangTidyMain.cpp itself... code-flow may look...

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 " << ConfigFile << ": " << EC.message() << "\n";
}

and then

Configassign(ConfigFile);
from then onwards it will be considered as Config values

Let me try implementing and test...

Hiralo updated this revision to Diff 300980.Oct 27 2020, 7:13 AM

With updated patch now changes are only in ClangTidyMain.cpp.
Thanks to @DmitryPolukhin for valuable suggestion :)

Added '--config-file' option to specify custom config file.
ClangTidyMain.cpp reads ConfigFile into string and then assigned read data to 'Config' i.e. makes like '--config' code flow internally.

Ref comments https://reviews.llvm.org/D89936#inline-839224

Thank you.
-Hiral

Yes, it is what I meant as a simpler solution. But please add Lit test even for such trivial option.

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

I suggest creating new local variable with text of the config from Config or ConfigFile file content i.e. avoid modifying Config itself.

Hiralo updated this revision to Diff 301044.Oct 27 2020, 10:02 AM

Added Lit test for --config-file cmdline option.

Hiralo updated this revision to Diff 301047.Oct 27 2020, 10:05 AM

Latest patch with lit test.

Yes, it is what I meant as a simpler solution. But please add Lit test even for such trivial option.

Please review latest patch and Lit test-case.

clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
2 ↗(On Diff #301047)

If you plan on contributing quite a lot then it would be wise to upload your patches with arcanist - https://llvm.org/docs/Phabricator.html. It will help prevent issues with diffs being relative to previous revisions.
Personally I just create a branch from master for a feature. When I create the patch or update, I just need to use arc diff master and it will handle everything for me.
The other advantage of using arc for patches is the pre-merge bot is then able to build and check your patch to make sure all tests pass.

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

Given you've removed the references to ConfigFile in ClangTidyOptions.h this should surely result in a compiler error, same goes below.

Hiralo updated this revision to Diff 301065.Oct 27 2020, 11:28 AM

Latest patch with Lit test-case.

Hiralo marked 4 inline comments as done.Oct 27 2020, 11:30 AM
Hiralo added inline comments.
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
289

Thanks for heads-up and references.

clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
2 ↗(On Diff #301047)

sure... in latest update added clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file

Hiralo marked 2 inline comments as done.Oct 27 2020, 11:30 AM

If you plan on contributing quite a lot then it would be wise to upload your patches with arcanist - https://llvm.org/docs/Phabricator.html. It will help prevent issues with diffs being relative to previous revisions.
Personally I just create a branch from master for a feature. When I create the patch or update, I just need to use arc diff master and it will handle everything for me.
The other advantage of using arc for patches is the pre-merge bot is then able to build and check your patch to make sure all tests pass.

Sure thanks for info.

will there be any update in this review after build error or successful merge ?

I think this diff looks very close to what we need. I hope it will be the last iteration.

Please also update title and description with the new name of the option, etc.

clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
2 ↗(On Diff #301065)

Please use FileCheck as all other tests do instead of grep to make test more portable. Also with FileCheck it should be no need in REQUIRES: shell.

And I think it is better to use -dump-config instead of checking specific check on a source file. This way you will tests only functionality that you implemented, without dependency some check behavior. Also it could be just another tests case in clang-tidy/infrastructure/config-files.cpp.

Hiralo added inline comments.Oct 28 2020, 6:55 AM
clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
2 ↗(On Diff #301065)

Please use FileCheck as all other tests do instead of grep to make test more portable. Also with FileCheck it should be no need in REQUIRES: shell.

And I think it is better to use -dump-config instead of checking specific check on a source file. This way you will tests only functionality that you implemented, without dependency some check behavior. Also it could be just another tests case in clang-tidy/infrastructure/config-files.cpp.

Sure...

Tried following...

// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config -- | FileCheck %s -check-           prefix=CHECK-BASE
// CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file %S/config-file.cpp -- | FileCheck %s -check-     prefix=CHECK-CHILD1
// CHECK-CHILD1: Checks: {{.*}}warning: integer literal has suffix 'ull', which is not uppercase [hicpp-uppercase- literal-suffix]  ########### This one fails!

Can you please suggest how to match with following o/p:

path/clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp:7:21: warning: integer literal has suffix 'ull', which is not uppercase [hicpp-uppercase-literal-suffix]
  unsigned long c = 100ull;
                    ^  ~~~
                       ULL

JFYI: also tried matching just 'ULL'!

Hiralo marked an inline comment as done.Oct 28 2020, 7:12 AM
Hiralo added inline comments.
clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
2 ↗(On Diff #301065)

Removed below test...
// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file %S/config-file.cpp -- | FileCheck %s -check- prefix=CHECK-CHILD1

Hiralo updated this revision to Diff 301277.Oct 28 2020, 7:23 AM
Hiralo retitled this revision from [clang-tidy] adding "--clang-tidy-config=<file-path>" to specify custom config file to [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Hiralo edited the summary of this revision. (Show Details)

Latest patch with Lit test-case.

Updated Title and Summary.

I think this diff looks very close to what we need. I hope it will be the last iteration.
Please also update title and description with the new name of the option, etc.

Updated latest patch :)

DmitryPolukhin added inline comments.Oct 29 2020, 3:13 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
336

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?

Hiralo added inline comments.Oct 29 2020, 3:57 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
336

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

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
322–326

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

336

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

It might be misleading what "InheritParentConfig: true" in the file means in this case. Current implementation that it will use directory structure of the source file and I think it is right approach. I would prefer to keep things simple and straightforward: --config-file should be file and it is simple helper for --config for the cases when you cannot use shell power to read file content to the command line or you exceed limit on command line length.

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
322–326

+1

DmitryPolukhin added inline comments.Oct 29 2020, 4:37 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
322–326

Clarify: +1 to @njames93 that we don't need this check.

Hiralo added inline comments.Oct 29 2020, 4:48 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
322–326

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

336

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.

DmitryPolukhin added inline comments.Oct 29 2020, 4:52 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
322–326

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.

Hiralo updated this revision to Diff 301845.Oct 30 2020, 3:05 AM

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

Let clang-tidy to read config from specified file.

This option internally works exactly the same way as --config option
after reading specified config file.

This option works as helper for --config in following few cases:

  • when there is shell limitation to read file content to the command line,
  • when there is limitation to command line length,
  • setting right build dependencies, and
  • others

--config-file and --config options are made mutually exclusive.

Example usage:
$ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
...this will read config from /some/path/myTidyConfigFile and

avoid searching '.clang-tidy' in parent-dirs.

May speed-up tidy runtime since now it will just look-up <file-path>
instead of searching ".clang-tidy" in parent-dirs.

Reviewed By: DmitryPolukhin, njames93

Differential Revision: https://reviews.llvm.org/D89936

Hiralo marked 6 inline comments as done.Oct 30 2020, 3:20 AM
Hiralo added inline comments.
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
322–326

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.

336

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

336

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

Hiralo marked 2 inline comments as done.Oct 30 2020, 3:21 AM
njames93 added inline comments.Oct 30 2020, 4:08 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
329–330

nit: Should we be using Config(File)?.getNumOccurances() > 0 here and below
If someone puts --config="" They are specifying a config(file)?, even if its empty.

Hiralo added inline comments.Oct 30 2020, 4:54 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
329–330

nit: Should we be using Config(File)?.getNumOccurances() > 0 here and below
If someone puts --config="" They are specifying a config(file)?, even if its empty.

Sounds good.

What is expected o/p with -config="" ?

Currently it works as below...
$ ./myinstall/bin/clang-tidy -config="" --dump-config --
config_occurances =1
config_is_empty = yes

Checks:          'clang-diagnostic-*,clang-analyzer-*'

WarningsAsErrors: ''
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle:     none
CheckOptions:
  - key:             llvm-else-after-return.WarnOnConditionVariables
...
Hiralo updated this revision to Diff 301862.Oct 30 2020, 5:15 AM

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

Let clang-tidy to read config from specified file.

This option internally works exactly the same way as --config option
after reading specified config file.

This option works as helper for --config in following few cases:

  • when there is shell limitation to read file content to the command line,
  • when there is limitation to command line length,
  • setting right build dependencies, and
  • others

--config-file and --config options are made mutually exclusive.

Example usage:
$ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
...this will read config from /some/path/myTidyConfigFile and

avoid searching '.clang-tidy' in parent-dirs.

May speed-up tidy runtime since now it will just look-up <file-path>
instead of searching ".clang-tidy" in parent-dirs.

Reviewed By: DmitryPolukhin, njames93

Differential Revision: https://reviews.llvm.org/D89936

Hiralo marked an inline comment as done.Oct 30 2020, 5:16 AM
Hiralo added inline comments.
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
329–330

nit: Should we be using Config(File)?.getNumOccurances() > 0 here and below
If someone puts --config="" They are specifying a config(file)?, even if its empty.

Updated patch...

$ ./myinstall/bin/clang-tidy --config="" --config-file="" --dump-config --
Error: --config-file and --config are mutually exclusive. Specify only one.

Thanks,

Hiralo added inline comments.Oct 30 2020, 5:20 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
329–330

nit: Should we be using Config(File)?.getNumOccurances() > 0 here and below
If someone puts --config="" They are specifying a config(file)?, even if its empty.

Updated patch...

$ ./myinstall/bin/clang-tidy --config="" --config-file="" --dump-config --
Error: --config-file and --config are mutually exclusive. Specify only one.

Thanks,

and

$ ./myinstall/bin/clang-tidy --config-file="" --dump-config --
Error: can't read config-file '': No such file or directory

DmitryPolukhin accepted this revision.Oct 30 2020, 8:27 AM

Looks good to me.

This revision is now accepted and ready to land.Oct 30 2020, 8:27 AM
Hiralo added a comment.Nov 2 2020, 6:17 PM

Looks good to me.

Thanks @DmitryPolukhin and @njames93 for your help.

This revision is now accepted and ready to land.Fri, Oct 30, 8:57 PM

When this patch will be merged and available in master?

When this patch will be merged and available in master?

@Hiralo I can push this changes to master for you. Please let me know if you need it.

Hiralo added a comment.Nov 3 2020, 1:51 AM

When this patch will be merged and available in master?

@Hiralo I can push this changes to master for you. Please let me know if you need it.

Yes please.

And just for confirmation, this patch will be part of the release 12.0.0 (i.e. Dec release), correct?

Hiralo added a comment.Nov 3 2020, 8:38 AM

Hello @@DmitryPolukhin ,

When I submitted latest via 'arc diff' my commit-message was...
Can you please help to have following commit message? Below commit message is more clear and helpful.
Sorry for inconvenience caused!

commit 653cb7912bbe4daabc8d6dd6dca71b2cf9a10365 (HEAD -> master)
Author: Hiral Oza <hiral.oza@netapp.com>
Date: Fri Oct 30 05:07:19 2020 -0400

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

Let clang-tidy to read config from specified file.

This option internally works exactly the same way as --config option
after reading specified config file.

This option works as helper for --config in following few cases:
 - when there is shell limitation to read file content to the command line,
 - when there is limitation to command line length,
 - setting right build dependencies, and
 - others

--config-file and --config options are made mutually exclusive.

Example usage:
 $ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
 ...this will read config from /some/path/myTidyConfigFile and
    avoid searching '.clang-tidy' in parent-dirs.

May speed-up tidy runtime since now it will just look-up <file-path>
instead of searching ".clang-tidy" in parent-dirs.

Reviewed By: DmitryPolukhin, njames93

Differential Revision: https://reviews.llvm.org/D89936
Hiralo edited the summary of this revision. (Show Details)Nov 3 2020, 8:40 AM

Hello @@DmitryPolukhin ,

When I submitted latest via 'arc diff' my commit-message was...
Can you please help to have following commit message? Below commit message is more clear and helpful.
Sorry for inconvenience caused!

It is not possible to update commit message afterwords. I received commit message from arc patch your diff (https://llvm.org/docs/Phabricator.html#committing-someone-s-change-from-phabricator). The only thing that I did manually was removing extra spaces and second link to the same codereview. I think arc patch ignores upload messages and uses diff description from Phabricator. Upload message just describes what you have changed in comparison with previous upload so it is expected behaviour.

Hiralo added a comment.Nov 3 2020, 4:51 PM

Hello @DmitryPolukhin ,
Sorry I missed to update 'SUMMARY' section of this review earlier! was not aware that that will be considered as commit message.
I have now updated the 'SUMMARY', it is possible to revert this commit and then re-submit it with updated commit message?
I think latest commit message reflects and provides details about usage and helpful to people.
Sorry for inconvenience caused!

Sorry, I don't think it's worth reverting and resubmitting the same changes in such a case. Commit attribution is correct and there is link to this code review.
After all, most clang-tidy users will never read commit messages and will only use --help for discovering this feature.

Hiralo added a comment.Nov 4 2020, 8:15 AM

Got it. No worries :)
Thanks @DmitryPolukhin for your help.