Page MenuHomePhabricator

Hiralo (Hiral)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 19 2020, 2:06 AM (5 w, 5 d)

Recent Activity

Wed, Nov 4

Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Wed, Nov 4, 8:15 AM · Restricted Project, Restricted Project

Tue, Nov 3

Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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!

Tue, Nov 3, 4:51 PM · Restricted Project, Restricted Project
Hiralo updated the summary of D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Tue, Nov 3, 8:40 AM · Restricted Project, Restricted Project
Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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!

Tue, Nov 3, 8:38 AM · Restricted Project, Restricted Project
Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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.

Tue, Nov 3, 1:51 AM · Restricted Project, Restricted Project

Mon, Nov 2

Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

Looks good to me.

Mon, Nov 2, 6:17 PM · Restricted Project, Restricted Project

Fri, Oct 30

Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Fri, Oct 30, 5:20 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Fri, Oct 30, 5:16 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Fri, Oct 30, 5:15 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Fri, Oct 30, 4:54 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Fri, Oct 30, 3:20 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Fri, Oct 30, 3:05 AM · Restricted Project, Restricted Project

Oct 29 2020

Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 29 2020, 4:48 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 29 2020, 3:57 AM · Restricted Project, Restricted Project

Oct 28 2020

Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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.

Oct 28 2020, 7:26 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

Latest patch with Lit test-case.

Oct 28 2020, 7:23 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 28 2020, 7:12 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 28 2020, 6:55 AM · Restricted Project, Restricted Project

Oct 27 2020

Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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.

Oct 27 2020, 11:34 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 27 2020, 11:30 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

Latest patch with Lit test-case.

Oct 27 2020, 11:28 AM · Restricted Project, Restricted Project
Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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.

Oct 27 2020, 10:07 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

Latest patch with lit test.

Oct 27 2020, 10:05 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Oct 27 2020, 10:02 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Oct 27 2020, 7:13 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 27 2020, 5:29 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 27 2020, 5:16 AM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 27 2020, 5:08 AM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

Another observation w.r.t. stdout...

Oct 27 2020, 1:42 AM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

JFYI: using --quiet avoids call to following write calls...so that is useful.
write(2, "Suppressed ", 11) = 11
write(2, "10703", 5) = 5
write(2, " warnings (", 11) = 11
write(2, "10703", 5) = 5
write(2, " in non-user code", 17) = 17
write(2, ").\n", 3) = 3
write(2, "Use -header-filter=.* to display"..., 136) = 136

Oct 27 2020, 1:35 AM · Restricted Project, Restricted Project
Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

...I think this diff needs some improvements

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

Oct 27 2020, 1:14 AM · Restricted Project, Restricted Project
Hiralo updated the diff for D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Oct 27 2020, 12:58 AM · Restricted Project, Restricted Project

Oct 26 2020

Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 26 2020, 11:42 PM · Restricted Project, Restricted Project
Hiralo added inline comments to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 26 2020, 11:30 PM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

By the looks of the code, you may want to call SetBufferSize only (do not call SetBuffered after that - or it'll go back to the default buffer size of 0.

Oct 26 2020, 10:16 PM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

Looks like you might be able to do something like "llvm::errs().setBuffered()" ?

Do we need to set it for each function using llvm:errs() (e.g. here printStats() )
OR can it be set once for entire clang-tidy ?

I think it could be set for the whole process. (though there's a reason stderr isn't usually buffered - because if it's buffered and the process crashes, then you don't get all the output/maybe something important will be lost)

Oct 26 2020, 9:52 PM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

Tried using llvm::errs().SetBuffered() within printStats()...

Oct 26 2020, 9:04 PM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

Looks like you might be able to do something like "llvm::errs().setBuffered()" ?

Oct 26 2020, 8:39 PM · Restricted Project, Restricted Project
Hiralo added a comment to D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..

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

Oct 26 2020, 8:25 PM · Restricted Project, Restricted Project
Hiralo added a comment to D90010: clang-tidy: Reduce number of stderr write calls.

Isn't llvm::errs() buffered, negating most of the benefit here.

Oct 26 2020, 8:20 PM · Restricted Project, Restricted Project

Oct 22 2020

Hiralo requested review of D90010: clang-tidy: Reduce number of stderr write calls.
Oct 22 2020, 10:30 PM · Restricted Project, Restricted Project
Hiralo requested review of D89936: [clang-tidy] adding "--config-file=<file-path>" to specify custom config file..
Oct 22 2020, 12:13 AM · Restricted Project, Restricted Project