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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/tool/clang-tidy-diff.py:123-130
   if args.fix:
     command.append('-fix')
   if args.checks != '':
     command.append('-checks=' + quote + args.checks + quote)
   if args.quiet:
     command.append('-quiet')
   if args.build_path is not None:
----------------
janosimas wrote:
> alexfh wrote:
> > janosimas wrote:
> > > janosimas wrote:
> > > > alexfh wrote:
> > > > > 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.
> > > > What about keep the current `--` behavior and add a new flag `-extra-tidy-flags` ? 
> > > `-extra-tidy-arg` to maintain consistency.
> > What's the benefit of `-extra-tidy-arg` compared to pass-by arguments?
> `-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.
> 
> -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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864