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

[Jano Simas via Phabricator via cfe-commits]

janosimas created this revision.
janosimas added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864

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


Index: clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tidy/tool/clang-tidy-diff.py
+++ clang-tidy/tool/clang-tidy-diff.py
@@ -70,7 +70,7 @@
   clang_tidy_args = []
   argv = sys.argv[1:]
   if '--' in argv:
-clang_tidy_args.extend(argv[argv.index('--'):])
+clang_tidy_args.extend(argv[argv.index('--')+1:])
 argv = argv[:argv.index('--')]
 
   args = parser.parse_args(argv)


Index: clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tidy/tool/clang-tidy-diff.py
+++ clang-tidy/tool/clang-tidy-diff.py
@@ -70,7 +70,7 @@
   clang_tidy_args = []
   argv = sys.argv[1:]
   if '--' in argv:
-clang_tidy_args.extend(argv[argv.index('--'):])
+clang_tidy_args.extend(argv[argv.index('--')+1:])
 argv = argv[:argv.index('--')]
 
   args = parser.parse_args(argv)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas requested review of this revision.
janosimas added inline comments.



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:

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added inline comments.



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:
> > 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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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?




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:

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.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 298865.
janosimas added a comment.

Here a diff with the rebased code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

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

Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -119,12 +119,13 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   '\nclang-tidy arguments should be passed after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing N slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -136,32 +137,14 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-path', dest='build_path',
-  help='Path used to read a compile command database.')
   if yaml:
 parser.add_argument('-export-fixes', metavar='FILE', dest='export_fixes',
 help='Create a yaml file to store suggested fixes in, '
 'which can be applied with clang-apply-replacements.')
-  parser.add_argument('-extra-arg', dest='extra_arg',
-  action='append', default=[],
-  help='Additional argument to append to the compiler '
-  'command line.')
-  parser.add_argument('-extra-arg-before', dest='extra_arg_before',
-  action='append', default=[],
-  help='Additional argument to prepend to the compiler '
-  'command line.')
-  parser.add_argument('-quiet', action='store_true', default=False,
-  help='Run clang-tidy in quiet mode')
   clang_tidy_args = []
   argv = sys.argv[1:]
   if '--' in argv:
-clang_tidy_args.extend(argv[argv.index('--'):])
+clang_tidy_args.extend(argv[argv.index('--')+1:])
 argv = argv[:argv.index('--')]
 
   args = parser.parse_args(argv)
@@ -170,7 +153,7 @@
   filename = None
   lines_by_file = {}
   for line in sys.stdin:
-match = re.search('^\+\+\+\ \"?(.*?/){%s}([^ \t\n\"]*)' % args.p, line)
+match = re.search('^\+\+\+\ \"?(.*?/){%s}([^ \t\n\"]*)' % args.strip, line)
 if match:
   filename = match.group(2)
 if filename is None:
@@ -215,20 +198,6 @@
   # Run a pool of clang-tidy workers.
   start_workers(max_task_count, run_tidy, task_queue, lock, args.timeout)
 
-  # Form the common args list.
-  common_clang_tidy_args = []
-  if args.fix:
-common_clang_tidy_args.append('-fix')
-  if args.checks != '':
-common_clang_tidy_args.append('-checks=' + args.checks)
-  if args.quiet:
-common_clang_tidy_args.append('-quiet')
-  if args.build_path is not None:
-common_clang_tidy_args.append('-p=%s' % args.build_path)
-  for arg in args.extra_arg:
-common_clang_tidy_args.append('-extra-arg=%s' % arg)
-  for arg in args.extra_arg_before:
-common_clang_tidy_args.append('-extra-arg-before=%s' % arg)
 
   for name in lines_by_file:
 line_filter_json = json.dumps(
@@ -244,7 +213,6 @@
   (handle, tmp_name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir)
   os.close(handle)
   command.append('-export-fixes=' 

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 347459.
janosimas added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

Files:
  clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
@@ -1,11 +1,11 @@
 // REQUIRES: shell
 // RUN: sed 's/placeholder_for_f/f/' %s > %t.cpp
-// RUN: clang-tidy -checks=-*,modernize-use-override %t.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
+// RUN: clang-tidy -- -checks=-*,modernize-use-override %t.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
 // RUN: mkdir -p %T/compilation-database-test/
 // RUN: echo '[{"directory": "%T", "command": "clang++ -o test.o -std=c++11 %t.cpp", "file": "%t.cpp"}]' > %T/compilation-database-test/compile_commands.json
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
 struct A {
   virtual void f() {}
   virtual void g() {}
Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -119,12 +119,13 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   '\nclang-tidy arguments should be passed after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing NUM slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -136,34 +137,14 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-use-color', action='store_true',
-  help='Use colors in output')
-  parser.add_argument('-path', dest='build_path',
-  help='Path used to read a compile command database.')
   if yaml:
 parser.add_argument('-export-fixes', metavar='FILE', dest='export_fixes',
 help='Create a yaml file to store suggested fixes in, '
 'which can be appli

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 347489.
janosimas added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

Files:
  clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
@@ -1,11 +1,11 @@
 // REQUIRES: shell
 // RUN: sed 's/placeholder_for_f/f/' %s > %t.cpp
 // RUN: clang-tidy -checks=-*,modernize-use-override %t.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
 // RUN: mkdir -p %T/compilation-database-test/
 // RUN: echo '[{"directory": "%T", "command": "clang++ -o test.o -std=c++11 %t.cpp", "file": "%t.cpp"}]' > %T/compilation-database-test/compile_commands.json
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
 struct A {
   virtual void f() {}
   virtual void g() {}
Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -119,12 +119,13 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   '\nclang-tidy arguments should be passed after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing NUM slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -136,34 +137,14 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-use-color', action='store_true',
-  help='Use colors in output')
-  parser.add_argument('-path', dest='build_path',
-  help='Path used to read a compile command database.')
   if yaml:
 parser.add_argument('-export-fixes', metavar='FILE', dest='export_fixes',
 help='Create a yaml file to store suggested fixes in, '
 'which can be applied with clang-apply-replacements.')
-  parser.add_argument('-extra-arg', dest='extra_arg',
-  action='append', default=[],
-  help='Additional argument to append to the compiler '
-  'comm

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 347622.
janosimas added a comment.

Fix test issue.

Use correct clang-tidy flag


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

Files:
  clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
@@ -1,11 +1,11 @@
 // REQUIRES: shell
 // RUN: sed 's/placeholder_for_f/f/' %s > %t.cpp
 // RUN: clang-tidy -checks=-*,modernize-use-override %t.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
 // RUN: mkdir -p %T/compilation-database-test/
 // RUN: echo '[{"directory": "%T", "command": "clang++ -o test.o -std=c++11 %t.cpp", "file": "%t.cpp"}]' > %T/compilation-database-test/compile_commands.json
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -p=%T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
 struct A {
   virtual void f() {}
   virtual void g() {}
Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -119,12 +119,13 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   '\nclang-tidy arguments should be passed after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing NUM slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -136,34 +137,14 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-use-color', action='store_true',
-  help='Use colors in output')
-  parser.add_argument('-path', dest='build_path',
-  help='Path used to read a compile command database.')
   if yaml:
 parser.add_argument('-export-fixes', metavar='FILE', dest='export_fixes',
 help='Create a yaml file to store suggested fixes in, '
 'which can be applied with clang-apply-replacements.')
-  parser.add_argument('-extra-arg', dest='extra_arg',
-  action='append', default=[],
-  help='Additional argument to append to the compiler '
-  'command line.')
-  parser.add_argument('-extra-

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 234005.
janosimas added a comment.
Herald added a subscriber: mgehre.
Herald added a project: clang.

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


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

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

Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1 -- -fix -checks=-*,modernize-use-override
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -118,12 +118,13 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   '\nclang-tidy arguments should be passed after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing N slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -135,32 +136,15 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-path', dest='build_path',
-  help='Path used to read a compile command database.')
   if yaml:
 parser.add_argument('-export-fixes', metavar='FILE', dest='export_fixes',
 help='Create a yaml file to store suggested fixes in, '
 'which can be applied with clang-apply-replacements.')
-  parser.add_argument('-extra-arg', dest='extra_arg',
-  action='append', default=[],
-  help='Additional argument to append to the compiler '
-  'command line.')
-  parser.add_argument('-extra-arg-before', dest='extra_arg_before',
-  action='append', default=[],
-  help='Additional argument to prepend to the compiler '
-  'command line.')
-  parser.add_argument('-quiet', action='store_true', default=False,
-  help='Run clang-tidy in quiet mode')
+
   clang_tidy_args = []
   argv = sys.argv[1:]
   if '--' in argv:
-clang_tidy_args.extend(argv[argv.index('--'):])
+clang_tidy_args.extend(argv[argv.index('--')+1:])
 argv = argv[:argv.index('--')]
 
   args = parser.parse_args(argv)
@@ -169,7 +153,7 @@
   filename = None
   lines_by_file = {}
   for line in sys.stdin:
-match = re.search('^\+\+\+\ \"?(.*?/){%s}([^ \t\n\"]*)' % args.p, line)
+match = re.search('^\+\+\+\ \"?(.*?/){%s}([^ \t\n\"]*)' % args.strip, line)
 if match:
   filename = match.group(2)
 if filename is None:
@@ -216,19 +200,6 @@
 
   # Form the common args list.
   common_clang_tidy_args = []
-  if args.fix:
-common_clang_tidy_args.append('-fix')
-  if args.checks != '':
-common_clang_tidy_args.append('-checks=' + args.checks)
-  if args.quiet:
-common_clang_tidy_args.append('-quiet')
-  if args.build_path is not None:
-common_clang_tidy_args.append('-p=%s' % args.build_path)
-  for arg in args.extra_arg:
-common_clang_tidy_args.append('-extra-arg=%s' % arg)
-  for arg in args.extra_arg_before:
-common_clang_tidy_args.append('-extra-arg-before=%s' % arg)
-
   for name in lines_by_file:
 line_filter_json = json.dumps(
   [{"n

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

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


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 400854.
janosimas added a comment.

I changed the wording and added an example as suggested.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

Files:
  clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
@@ -1,11 +1,11 @@
 // REQUIRES: shell
 // RUN: sed 's/placeholder_for_f/f/' %s > %t.cpp
 // RUN: clang-tidy -checks=-*,modernize-use-override %t.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
 // RUN: mkdir -p %T/compilation-database-test/
 // RUN: echo '[{"directory": "%T", "command": "clang++ -o test.o -std=c++11 %t.cpp", "file": "%t.cpp"}]' > %T/compilation-database-test/compile_commands.json
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -p=%T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
 struct A {
   virtual void f() {}
   virtual void g() {}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -79,6 +79,8 @@
 - Generalized the `modernize-use-default-member-init` check to handle non-default
   constructors.
 
+- `clang-tidy-diff.py` allows using `clang-tidy` flags after `--`. This is a breaking change, check the script help for usage.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -119,12 +119,14 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   ' clang-tidy arguments can be passed after a \'--\'.'
+   ' Ex.: \'git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1 -- -fix -checks=-*,modernize-use-override\'')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing NUM slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -136,34 +138,14 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-use-color', action='store_true',
-

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

[Jano Simas via Phabricator via cfe-commits]

janosimas marked 2 inline comments as done.
janosimas added a comment.

Yes, I'll need help landing it


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jano Simas via Phabricator via cfe-commits]

janosimas updated this revision to Diff 398314.
janosimas added a comment.
Herald added a subscriber: carlosgalvezp.

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


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

Files:
  clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-diff.cpp
@@ -1,11 +1,11 @@
 // REQUIRES: shell
 // RUN: sed 's/placeholder_for_f/f/' %s > %t.cpp
 // RUN: clang-tidy -checks=-*,modernize-use-override %t.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -- -std=c++11 2>&1 | FileCheck %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -quiet -- -std=c++11 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
 // RUN: mkdir -p %T/compilation-database-test/
 // RUN: echo '[{"directory": "%T", "command": "clang++ -o test.o -std=c++11 %t.cpp", "file": "%t.cpp"}]' > %T/compilation-database-test/compile_commands.json
-// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -checks=-*,modernize-use-override -path %T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
+// RUN: not diff -U0 %s %t.cpp | %clang_tidy_diff -- -checks=-*,modernize-use-override -p=%T/compilation-database-test 2>&1 | FileCheck -check-prefix=CHECK %s
 struct A {
   virtual void f() {}
   virtual void g() {}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -79,6 +79,8 @@
 - Generalized the `modernize-use-default-member-init` check to handle non-default
   constructors.
 
+- `clang-tidy-diff.py` allows using `clang-tidy` flags after `--`. This is a breaking change, check the script help for usage.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
===
--- clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
+++ clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py
@@ -17,9 +17,9 @@
 detect clang-tidy regressions in the lines touched by a specific patch.
 Example usage for git/svn users:
 
-  git diff -U0 HEAD^ | clang-tidy-diff.py -p1
+  git diff -U0 HEAD^ | clang-tidy-diff.py -strip 1
   svn diff --diff-cmd=diff -x-U0 | \
-  clang-tidy-diff.py -fix -checks=-*,modernize-use-override
+  clang-tidy-diff.py -- -fix -checks=-*,modernize-use-override
 
 """
 
@@ -119,12 +119,13 @@
   parser = argparse.ArgumentParser(description=
'Run clang-tidy against changed files, and '
'output diagnostics only for modified '
-   'lines.')
+   'lines.'
+   '\nclang-tidy arguments should be passed after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
-  parser.add_argument('-p', metavar='NUM', default=0,
-  help='strip the smallest prefix containing P slashes')
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing NUM slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,
   help='custom pattern selecting file paths to check '
   '(case sensitive, overrides -iregex)')
@@ -136,34 +137,14 @@
   help='number of tidy instances to be run in parallel.')
   parser.add_argument('-timeout', type=int, default=None,
   help='timeout per each file in seconds.')
-  parser.add_argument('-fix', action='store_true', default=False,
-  help='apply suggested fixes')
-  parser.add_argument('-checks',
-  help='checks filter, when not specified, use clang-tidy '
-  'default',
-  default='')
-  parser.add_argument('-use-color', action='store_true',
-  help='Use colors in output')
-  parser.add_argument('-path', dest='build_path',
-  

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

[Jano Simas via Phabricator via cfe-commits]

janosimas added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49864/new/

https://reviews.llvm.org/D49864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits