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

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.Oct 12 2020, 6:42 AM
This revision now requires changes to proceed.Oct 12 2020, 6:42 AM
janosimas updated this revision to Diff 298865.Oct 18 2020, 2:41 AM

Here a diff with the rebased code

alexfh requested changes to this revision.Jan 28 2021, 5:57 PM

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

This revision now requires changes to proceed.Jan 28 2021, 5:57 PM

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?

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.

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

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?

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

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.

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 updated this revision to Diff 347489.May 24 2021, 1:39 PM

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

janosimas updated this revision to Diff 347622.May 25 2021, 3:39 AM

Fix test issue.

Use correct clang-tidy flag

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.

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

alexfh added a comment.Fri, Oct 1, 4:08 PM

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.