[PATCH] D53798: [lit] Add --show-substitutions

[serge via Phabricator via llvm-commits]

serge-sans-paille added a comment.

Apart from the regexp pretty-printing, this LGTM.



================
Comment at: utils/lit/lit/main.py:432
+                    print('    Substitutions')
+                    for (pattern, replacement) in ts.config.substitutions:
+                        if re.match(r'[\'"]|.*\s', pattern):
----------------
[nits] the parenthesis around the tuple are not needed.


================
Comment at: utils/lit/lit/main.py:433
+                    for (pattern, replacement) in ts.config.substitutions:
+                        if re.match(r'[\'"]|.*\s', pattern):
+                            pattern = repr(pattern)
----------------
delcypher wrote:
> eush wrote:
> > delcypher wrote:
> > > Is all this regex really necessary? Couldn't we just always print `repr(pattern), repr(replacement)`?
> > My only concern is that otherwise it would be harder to read regexes since they would be (doubly) escaped.
> Then why even use `repr(...)`? Just print as a regular string always.
> 
> E.g. 
> ```
> >>> print(repr(r'\s\w'))
> '\\s\\w'
> >>> print(str(r'\s\w'))
> \s\w
> ```
> 
> I honestly don't understand what your regex logic is trying to achieve so I really think you should drop it.
I agree with @delcypher . Just printing the pattern « as is » should be fine.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53798