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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.
Herald added a reviewer: njames93.
Herald added a project: All.

This need to be rebase due to conflict to land.


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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Do you need help landing the change?


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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

Looks good with one suggestion.




Comment at: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py:123
+   'lines.'
+   '\nclang-tidy arguments should be passed 
after a \'--\' .')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',

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


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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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.


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.

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

[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 (--)

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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.


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

[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 '
-  

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D49864#274 , @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.


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.

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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.

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

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.




Comment at: clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py:128
+  parser.add_argument('-strip', metavar='NUM', default=0,
+  help='strip the smallest prefix containing N slashes')
   parser.add_argument('-regex', metavar='PATTERN', default=None,

s/N/NUM/ ?


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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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.


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

[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 (--)

[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



___
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 (--)

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh 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:
> 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?


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

[Alexander Kornienko via Phabricator via cfe-commits]

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

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.


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.

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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?


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