[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