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

[janosimas]

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.

[alexfh]

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?

[janosimas]

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.

[alexfh]

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.

[janosimas]

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]

clang-tidy/tool/clang-tidy-diff.py

123–130 ↗

(On Diff #157515)

-extra-tidy-arg to maintain consistency.

[alexfh]

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?

[janosimas]

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]

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.

[janosimas]

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]

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

[janosimas]

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

[alexfh]

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.

[janosimas]

Here a diff with the rebased code

[alexfh]

Apologies again for the long delay.

A diff with full context would still be appreciated. Please read https://llvm.org/docs/Phabricator.html for instructions.

The clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp test needs to be updated accordingly. See Diff Detail -> Build Status for the build results after uploading a patch to Phabricator.

clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py

128

s/N/NUM/ ?

[janosimas]

Hi, sorry for taking so long for such a small change.
I did the changes and generated a diff with the requested context.
It's a huge file with a lot more diff than my changes, is that right?

When I try to upload it, I'm getting Unhandled Exception ("Exception"), no other error message.
Any suggestions there?

[aaron.ballman]

In D49864#2776570, @janosimas wrote:

Hi, sorry for taking so long for such a small change.
I did the changes and generated a diff with the requested context.
It's a huge file with a lot more diff than my changes, is that right?

That sounds incorrect. The generated diff should be basically the same size as what's already in the review.

When I try to upload it, I'm getting Unhandled Exception ("Exception"), no other error message.
Any suggestions there?

That sounds like a Phabricator issue, but perhaps it has to do with an unexpectedly large patch file. I'd recommend first getting the patch file to the point it looks reasonable, and then if the Phab issues persist, we can investigate them at that point.

[janosimas]

I found the issue with my diff and I was able to get the expected file.

I'm still having issues uploading the file, this is the message I get in the browser console:

POST https://reviews.llvm.org/differential/revision/update/49864/ 500

[aaron.ballman]

In D49864#2777300, @janosimas wrote:

I found the issue with my diff and I was able to get the expected file.

I'm still having issues uploading the file, this is the message I get in the browser console:

POST https://reviews.llvm.org/differential/revision/update/49864/ 500

Hmm, I'm not certain what's going on there. When I try to upload a patch for the review, Phab lets me upload it (but I didn't try to submit the changes). At what stage are you getting the failure? Is it when uploading the patch itself, or at some other point?

[janosimas]

I manage to upload it, copy-pasting the content to the text box. I have no idea why it is not working with the upload.

Updates from the last change

• Modified the tests to match the input
• Addend file context

[janosimas]

Hmm, I'm not certain what's going on there. When I try to upload a patch for the review, Phab lets me upload it (but I didn't try to submit the changes). At what stage are you getting the failure? Is it when uploading the patch itself, or at some other point?

Immediately when I try to upload a diff file.
I fill the form with the file I want to upload, the repo and when I try to submit I get the error.

[aaron.ballman]

In D49864#2777774, @janosimas wrote:

I manage to upload it, copy-pasting the content to the text box. I have no idea why it is not working with the upload.

Updates from the last change

• Modified the tests to match the input
• Addend file context

Can confirm that the new diff looks to have not been munged somehow. Sorry that you had the weird Phab issues, hopefully they're transitory and won't crop up again on the review.

The changes seem reasonable to me, but you should wait for @alexfh to sign off on the patch.

[janosimas]

Fix a mistake in my last patch.
I added a -- in a command that should not have it.

[janosimas]

Fix test issue.

Use correct clang-tidy flag

[alexfh]

Function-wise the patch looks good, but I'm somewhat concerned about silently breaking users. At the very least there should be a relevant entry in the release notes.

[janosimas]

That makes sense.
Should I add it somewhere? Or do I need to talk to someone?

[janosimas]

ping

[alexfh]

In D49864#2857630, @janosimas wrote:

That makes sense.
Should I add it somewhere? Or do I need to talk to someone?

Please update clang-tools-extra/docs/ReleaseNotes.rst. It looks like there's no other docs for this script.

[janosimas]

I added a comment in the docs noting this is a breaking change.

[alexfh]

Looks good with one suggestion.

clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py

123

I'd say "can" instead of "should" (all defaults may be just fine depending on the setup) and maybe also add an example.

[alexfh]

Do you need help landing the change?

[janosimas]

I changed the wording and added an example as suggested.

[janosimas]

Yes, I'll need help landing it

[PiotrZSL]

This need to be rebase due to conflict to land.