[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog created this revision.
Herald added a subscriber: carlosgalvezp.
kesyog requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

D90110  modified the behavior of 
`run-clang-tidy` to always pass the
`--use-color` option to clang-tidy, which enabled colored diagnostics
output regardless of TTY status or .clang-tidy settings. This left the
user with no option to disable the colored output.

This presents an issue when trying to parse the output of run-clang-tidy
programmaticall, as the output is polluted with ANSI escape characters.

This PR fixes this issue in two ways:

1. It restores the default behavior of `run-clang-tidy` to let `clang-tidy` 
decide whether to color output. This allows the user to configure color via the 
`UseColor` option in a .clang-tidy file.
2. It adds mutually exclusive, optional `-use-color` and `-no-use-color` 
argument flags that let the user explicitly set the color option via the 
invocation.

After this change the default behavior of `run-clang-tidy` when no
.clang-tidy file is available is now to show no color, presumably
because `clang-tidy` detects that the output is being piped and defaults
to not showing colored output. This seems like an acceptable tradeoff
to respect .clang-tidy configurations, as users can still use the
`-use-color` option to explicitly enable color.

Fixes #49441 (50097 in Bugzilla)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119562

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


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -82,15 +82,19 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 extra_arg, extra_arg_before, quiet, config,
-line_filter):
+line_filter, use_color):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if line_filter is not None:
 start.append('-line-filter=' + line_filter)
+  if use_color:
+start.append('--use-color')
+  elif use_color is not None:
+start.append('--use-color=false')
   if checks:
 start.append('-checks=' + checks)
   if tmpdir is not None:
@@ -168,7 +172,8 @@
  tmpdir, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 
 proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
 output, err = proc.communicate()
@@ -231,6 +236,16 @@
   'after applying fixes')
   parser.add_argument('-style', default='file', help='The style of reformat '
   'code after applying fixes')
+  color_group = parser.add_mutually_exclusive_group()
+  color_group.add_argument('-use-color', action='store_true', dest='use_color',
+   help='Use colors in diagnostics, overriding 
clang-tidy\'s default '
+   'behavior. This option overrides the \'UseColor\' 
option in'
+   '.clang-tidy file, if any.')
+  color_group.add_argument('-no-use-color', action='store_false', 
dest='use_color',
+   help='Do not use colors in diagnostics, overriding 
clang-tidy\'s default'
+   ' behavior. This option overrides the \'UseColor\' 
option in'
+   '.clang-tidy file, if any.')
+  parser.set_defaults(use_color=None)
   parser.add_argument('-p', dest='build_path',
   help='Path used to read a compile command database.')
   parser.add_argument('-extra-arg', dest='extra_arg',
@@ -258,7 +273,8 @@
  None, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 invocation.append('-list-checks')
 invocation.ap

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:96
+start.append('--use-color')
+  elif use_color is not None:
+start.append('--use-color=false')

Eugene.Zelenko wrote:
> Shouldn't it be just `else:`?
There are three cases:

| Argument  | `use_color` | Behavior |
| `-use-color` | `True` | `--use-color` is passed to clang-tidy, force enabling 
color |
| `-no-use-color` | `False` | `--use-color=false` is passed to clang-tidy, 
force disabling color |
| (none provided) | `None` | Nothing passed to clang-tidy. clang-tidy follows 
its default coloring behavior |

The case on the highlighted line is the second row of the table, and we have to 
check that `use_color` is not `None` to exclude the case of the third row.

I was trying to avoid the extra nesting of something like below, but maybe the 
intent would be clearer?
```
if use_color is not None:
  if use_color:
start.append('--use-color')
  else:
start.append('--use-color=false')
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog updated this revision to Diff 407950.
kesyog added a comment.

Refactor tri-state logic for readability


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -82,15 +82,20 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 extra_arg, extra_arg_before, quiet, config,
-line_filter):
+line_filter, use_color):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if line_filter is not None:
 start.append('-line-filter=' + line_filter)
+  if use_color is not None:
+if use_color:
+  start.append('--use-color')
+else:
+  start.append('--use-color=false')
   if checks:
 start.append('-checks=' + checks)
   if tmpdir is not None:
@@ -168,7 +173,8 @@
  tmpdir, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 
 proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
 output, err = proc.communicate()
@@ -231,6 +237,16 @@
   'after applying fixes')
   parser.add_argument('-style', default='file', help='The style of reformat '
   'code after applying fixes')
+  color_group = parser.add_mutually_exclusive_group()
+  color_group.add_argument('-use-color', action='store_true', dest='use_color',
+   help='Use colors in diagnostics, overriding 
clang-tidy\'s default '
+   'behavior. This option overrides the \'UseColor\' 
option in'
+   '.clang-tidy file, if any.')
+  color_group.add_argument('-no-use-color', action='store_false', 
dest='use_color',
+   help='Do not use colors in diagnostics, overriding 
clang-tidy\'s default'
+   ' behavior. This option overrides the \'UseColor\' 
option in'
+   '.clang-tidy file, if any.')
+  parser.set_defaults(use_color=None)
   parser.add_argument('-p', dest='build_path',
   help='Path used to read a compile command database.')
   parser.add_argument('-extra-arg', dest='extra_arg',
@@ -258,7 +274,8 @@
  None, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 invocation.append('-list-checks')
 invocation.append('-')
 if args.quiet:


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -82,15 +82,20 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 extra_arg, extra_arg_before, quiet, config,
-line_filter):
+line_filter, use_color):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if line_filter is not None:
 start.append('-line-filter=' + line_filter)
+  if use_color is not None:
+if use_color:
+  start.append('--use-color')
+else:
+  start.append('--use-color=false')
   if checks:
 start.append('-checks=' + checks)
   if tmpdir is not None:
@@ -168,7 +173,8 @@
  tmpdir, build_pat

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog marked an inline comment as done.
kesyog added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:96
+start.append('--use-color')
+  elif use_color is not None:
+start.append('--use-color=false')

Eugene.Zelenko wrote:
> kesyog wrote:
> > Eugene.Zelenko wrote:
> > > Shouldn't it be just `else:`?
> > There are three cases:
> > 
> > | Argument  | `use_color` | Behavior |
> > | `-use-color` | `True` | `--use-color` is passed to clang-tidy, force 
> > enabling color |
> > | `-no-use-color` | `False` | `--use-color=false` is passed to clang-tidy, 
> > force disabling color |
> > | (none provided) | `None` | Nothing passed to clang-tidy. clang-tidy 
> > follows its default coloring behavior |
> > 
> > The case on the highlighted line is the second row of the table, and we 
> > have to check that `use_color` is not `None` to exclude the case of the 
> > third row.
> > 
> > I was trying to avoid the extra nesting of something like below, but maybe 
> > the intent would be clearer?
> > ```
> > if use_color is not None:
> >   if use_color:
> > start.append('--use-color')
> >   else:
> > start.append('--use-color=false')
> > ```
> I think you implementation is incorrect. You should check for not `None` 
> first and than set proper value for `--use-color`.
fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog marked an inline comment as done.
kesyog added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog updated this revision to Diff 410132.
kesyog added a comment.

Use argument style more consistent with clang-tidy & LLVM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -62,6 +62,21 @@
 import queue as queue
 
 
+def strtobool(val):
+  """Convert a string representation of truth to a bool following LLVM's CLI 
argument parsing."""
+
+  val = val.lower()
+  if val in ['', 'true', '1']:
+return True
+  elif val in ['false', '0']:
+return False
+
+  # Return ArgumentTypeError so that argparse does not substitute its own 
error message
+  raise argparse.ArgumentTypeError(
+"'{}' is invalid value for boolean argument! Try 0 or 1.".format(val)
+  )
+
+
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
@@ -82,15 +97,20 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 extra_arg, extra_arg_before, quiet, config,
-line_filter):
+line_filter, use_color):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if line_filter is not None:
 start.append('-line-filter=' + line_filter)
+  if use_color is not None:
+if use_color:
+  start.append('--use-color')
+else:
+  start.append('--use-color=false')
   if checks:
 start.append('-checks=' + checks)
   if tmpdir is not None:
@@ -168,7 +188,8 @@
  tmpdir, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 
 proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
 output, err = proc.communicate()
@@ -231,6 +252,10 @@
   'after applying fixes')
   parser.add_argument('-style', default='file', help='The style of reformat '
   'code after applying fixes')
+  parser.add_argument('-use-color', type=strtobool, nargs='?', const=True,
+  help='Use colors in diagnostics, overriding 
clang-tidy\'s'
+  ' default behavior. This option overrides the \'UseColor'
+  '\' option in .clang-tidy file, if any.')
   parser.add_argument('-p', dest='build_path',
   help='Path used to read a compile command database.')
   parser.add_argument('-extra-arg', dest='extra_arg',
@@ -258,7 +283,8 @@
  None, build_path, args.header_filter,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
- args.quiet, args.config, args.line_filter)
+ args.quiet, args.config, args.line_filter,
+ args.use_color)
 invocation.append('-list-checks')
 invocation.append('-')
 if args.quiet:


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -62,6 +62,21 @@
 import queue as queue
 
 
+def strtobool(val):
+  """Convert a string representation of truth to a bool following LLVM's CLI argument parsing."""
+
+  val = val.lower()
+  if val in ['', 'true', '1']:
+return True
+  elif val in ['false', '0']:
+return False
+
+  # Return ArgumentTypeError so that argparse does not substitute its own error message
+  raise argparse.ArgumentTypeError(
+"'{}' is invalid value for boolean argument! Try 0 or 1.".format(val)
+  )
+
+
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
@@ -82,15 +97,20 @@
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
 header_filter, allow_enabling_alpha_checkers,
 

[PATCH] D119562: Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog marked an inline comment as done.
kesyog added a comment.

Thanks for the suggestions. I removed `-no-use-color` and modified `-use-color` 
to follow the behavior of LLVM's CLI parser. I also fixed the (very 
unintentional) file mode change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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

[PATCH] D119562: [clang-tidy] Provide fine control of color in run-clang-tidy

[Kesavan Yogeswaran via Phabricator via cfe-commits]

kesyog added a comment.

I don't have commit privileges, so I'll need help committing this to `main`. My 
name is "Kesavan Yogeswaran" and my email is hikes [at] google [dot] com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119562

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