[PATCH] D92542: [tools] Update update_test_prefix.py to handle %s after prefixes

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/utils/update_test_prefix.py:21
         else:
-            s = re.sub('-?-check-prefixes=([^, ]+\n)', '--check-prefix=\\1', s)
-            s = re.sub('-?-check-prefixes=([^, ]+) ', '--check-prefix=\\1', s)
+            s = re.sub('-?-check-prefixes=([\w-]+[\s]*) ', '--check-prefix=\\1', s)
             t = re.search('-?-check-(?:prefix|prefixes)=([^ ]+)\s+-?-check-(?:prefix|prefixes)=([^ ]+)', s)
----------------
mtrofin wrote:
> mtrofin wrote:
> > pengfei wrote:
> > > mtrofin wrote:
> > > > pengfei wrote:
> > > > > I checked it and find it didn't work. It seems `\w` in `[]` loses its function.
> > > > > I also think the `*` after `[\s]` (you can use `\s` without `[]`) is problematical for `--check-prefixes=A,B`. Besides, changing to `+` is still problematical for it may consume extera blank lines.
> > > > > It's hard to modify regular expression correctly without lots of checks, I think you can use my previous patch as a test:
> > > > > ```
> > > > > git reset --hard c22dc71b120b066c0066b8517014149a001cc2b0^1
> > > > > git show c22dc71b120b066c0066b8517014149a001cc2b0 --stat | grep "llvm/test" | cut -d " " -f 2 | xargs llvm/utils/update_test_prefix.py
> > > > > ```
> > > > > Check if the outputs are identical to the patch.
> > > > The problem seems to have been a spurious space. I tried small unittests, like this:
> > > > 
> > > > ```
> > > > import re
> > > > 
> > > > test = ['-check-prefixes=abc123', '-check-prefixes=abc123-def', '-check-prefixes=abc %s', """-check-prefixes=abc
> > > >         def""", '-check-prefixes=abc,def', '-check-prefixes=abc,def %s']
> > > > 
> > > > for t in test:
> > > >   print(t, re.sub('-?-check-prefixes=([\w-]+[\s]*)', '--check-prefix=\\1', t))
> > > > 
> > > > ```
> > > > 
> > > > ```
> > > > $ python test.py
> > > > ('-check-prefixes=abc123', '--check-prefix=abc123')
> > > > ('-check-prefixes=abc123-def', '--check-prefix=abc123-def')
> > > > ('-check-prefixes=abc %s', '--check-prefix=abc %s')
> > > > ('-check-prefixes=abc\n        def', '--check-prefix=abc\n        def')
> > > > ('-check-prefixes=abc,def', '--check-prefix=abc,def')
> > > > ('-check-prefixes=abc,def %s', '--check-prefix=abc,def %s')
> > > > ```
> > > Ah, that's it. The unittest is awesome. But the last 2 test results are wrong. The expected results are:
> > > ```
> > > ('-check-prefixes=abc,def', '--check-prefixes=abc,def')
> > > ('-check-prefixes=abc,def %s', '--check-prefixes=abc,def %s')
> > > ```
> > > We can remove the `es` when there is only one prefix. You can add one more test:
> > > ```
> > > ('-check-prefixes=abc\n\n        def', '--check-prefix=abc\n\n        def')
> > > ```
> > > to see using `+` instead of `*` is also problematic.
> > One more try: 
> > 
> > print(t, re.sub('-?-check-prefixes=([\w-]+)(\Z|[ \t\n])', '--check-prefix=\\1\\2', t))
> > 
> > ```
> > ('-check-prefixes=abc123', '--check-prefix=abc123')
> > ('-check-prefixes=abc123-def', '--check-prefix=abc123-def')
> > ('-check-prefixes=abc %s', '--check-prefix=abc %s')
> > ('-check-prefixes=abc\n        def', '--check-prefix=abc\n        def')
> > ('-check-prefixes=abc,def', '-check-prefixes=abc,def')
> > ('-check-prefixes=abc,def %s', '-check-prefixes=abc,def %s')
> > ```
> oh, and (for the test you suggested)
> 
> ('-check-prefixes=abc\n\n        def', '--check-prefix=abc\n\n        def')
It looks good to me. Thanks Mircea!
I know why you are using such expression now. I missed the cases 1, 2 which aren't end up with `'\n'` or space, there may exist such cases in real tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92542