Page MenuHomePhabricator

[clang-tidy] The script clang-tidy-diff.py doesn't accept 'pass by' options (--)
Needs ReviewPublic

Authored by janosimas on Jul 26 2018, 9:57 AM.

Details

Summary

When the argument -- is passed to clang-tidy-diff.py it should pass the following arguments to clang-tidy.
It does that but also includes -- as an argument,
there should be a +1 in the '--' index.

So only the following arguments as passed to clang-tidy.

Diff Detail

Unit TestsFailed

TimeTest
140 mslinux > Clang Tools.clang-tidy/infrastructure::clang-tidy-diff.cpp
Script: -- : 'RUN: at line 2'; sed 's/placeholder_for_f/f/' /mnt/disks/ssd0/agent/llvm-project/clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp > /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/infrastructure/Output/clang-tidy-diff.cpp.tmp.cpp

Event Timeline

janosimas created this revision.Jul 26 2018, 9:57 AM
Eugene.Zelenko retitled this revision from The script clang-tidy-diff.py doesn't accept 'pass by' options (--) to [clang-tidy] The script clang-tidy-diff.py doesn't accept 'pass by' options (--).Jul 26 2018, 10:13 AM

This is intended, IIUC. The syntax of the clang-tidy-diff.py mirrors the syntax of clang-tidy itself, and the -- option is used in the same way as in clang-tidy - to denote the start of compiler arguments (and switch to the fixed compilation database). Do you have a use case that would require passing clang-tidy options beyond the list already supported by clang-tidy-diff.py?

alexfh requested changes to this revision.Sep 18 2018, 8:14 AM
This revision now requires changes to proceed.Sep 18 2018, 8:14 AM

To use in a git pre-commit I wanted to use the flags:
-warnings-as-errors=*
-header-filter=.*

I wanted that the script returned an error value for my selected checks, even if they are just warnings.

janosimas requested review of this revision.Sep 18 2018, 9:04 AM
alexfh added inline comments.Sep 24 2018, 5:55 AM
clang-tidy/tool/clang-tidy-diff.py
123–130 ↗(On Diff #157515)

If we make the script leave out the -- flag, we should stop forwarding these flags and the extra_arg(_before)? below. Otherwise it's too confusing (should one place -fix before -- or after? what about -warnings-as-errors?).

Please also update the usage example at the top.

alexfh requested changes to this revision.Sep 24 2018, 5:56 AM
This revision now requires changes to proceed.Sep 24 2018, 5:56 AM
janosimas requested review of this revision.Sep 26 2018, 10:44 AM
janosimas added inline comments.
clang-tidy/tool/clang-tidy-diff.py
123–130 ↗(On Diff #157515)

What about keep the current -- behavior and add a new flag -extra-tidy-flags ?

janosimas added inline comments.Sep 26 2018, 10:45 AM
clang-tidy/tool/clang-tidy-diff.py
123–130 ↗(On Diff #157515)

-extra-tidy-arg to maintain consistency.

alexfh added inline comments.Oct 1 2018, 5:14 AM
clang-tidy/tool/clang-tidy-diff.py
123–130 ↗(On Diff #157515)

What's the benefit of -extra-tidy-arg compared to pass-by arguments?

I was thinking about the usage of -- and -extra-arg, don't they do the same thing?
To be honest, for me, the current behavior of -- doesn't make much sense. If there is a use case for -extra-arg-before and -extra-arg, they are much more clearer in intent.
For me, -- usual behavior would be pass by options for the first next program, clang-tidy in this case.


Is there a reason to limit how the flags a passed to clang-tidy?
Why not pass all options as if using clang-tidy and only threat special cases and default values?

clang-tidy/tool/clang-tidy-diff.py
123–130 ↗(On Diff #157515)

-extra-tidy-arg would keep the current -- behavior and cover other cases of extra flags not supported by this script.
I was worried about how changing the behavior would affect people that already using it.

alexfh requested changes to this revision.Oct 1 2018, 8:16 AM
alexfh added inline comments.
clang-tidy/tool/clang-tidy-diff.py
123–130 ↗(On Diff #157515)

-extra-tidy-arg would keep the current -- behavior and cover other cases of extra flags not supported by this script.
I was worried about how changing the behavior would affect people that already using it.

Good point about backward compatibility, however it may be a good time to reset the complexity at the expense of breaking the interface. Luckily, call-sites will be easy to upgrade.

As you may have noticed, the system is already quite confusing. At the very beginning the script tried to mimic clang-tidy command-line interface with an addition of diff file parsing (which is translated to -line-filter=). Gradually, the script got more and more options - some of them were just forwarded to clang-tidy, some were needed for the script. Some options started conflicting (e.g. the script's -p and clang-tidy's -p) and had to be given different names (-path -> -p).

At this point adding another way to pass options to clang-tidy will just make this all even more confusing. So I'm suggesting to clearly split the script's options and clang-tidy's options:

clang-tidy-diff.py [-clang-tidy-binary ...] [-p ...] [-regex ...] [-iregex ...] -- [-fix] [-checks=...] [other clang-tidy options]

If there's a need to run clang-tidy with the fixed compilation database, the second -- will be needed, e.g.:

git diff -U0 HEAD^ | clang-tidy-diff.py -p1 -- -fix -- -std=c++11

The good thing is that script won't have to do anything extra to enable this and that it's very easy to explain: everything after the first -- is just passed to clang-tidy verbatim.

This revision now requires changes to proceed.Oct 1 2018, 8:16 AM

I like a lot of this syntax you proposed, makes a lot more sense to me.

The script will be shorter and the doc will be "auto updated" with the clang-tidy doc.

janosimas updated this revision to Diff 234005.Dec 16 2019, 2:15 AM

I reviewed the code over the discussion with the -- option,
I also changed the -p optin to -strip to avoid confusion with the clang-tidy option.

sorry for taking so long ;-)

Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2019, 2:15 AM
Herald added a subscriber: mgehre. · View Herald Transcript

I also noticed there is a clang-format-diff that also has the -p option, it would be nice to update it for consistency.

mgehre removed a subscriber: mgehre.Dec 16 2019, 11:04 AM

Sorry for the delay. This patch fell through the cracks. If you're still interested, could you rebase it on top of current HEAD and upload a full diff? Or use the arcanist tool, see https://llvm.org/docs/Phabricator.html.

alexfh requested changes to this revision.Mon, Oct 12, 6:42 AM
This revision now requires changes to proceed.Mon, Oct 12, 6:42 AM
janosimas updated this revision to Diff 298865.Sun, Oct 18, 2:41 AM

Here a diff with the rebased code