[PATCH] D28384: Update update_test_checks to work properly with phi nodes and other fun things.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 2 inline comments as done.
dberlin added a comment.

My strong suggestion would be for you to try your idea out :)
If you tried your original ssaify code, you'd immediately notice it deletes spaces and parens :)

This is one of the reasons i said the original code was simpler.

In any case, let me try to expand on  "sub expects to replace the entire match":

The entire match is group 0. Group 0 includes the match of the entire regexp, not just the capture groups (which start at 1).

  Python 2.7.6 (default, Oct 26 2016, 20:30:19) 
  [GCC 4.8.4] on linux2
  Type "help", "copyright", "credits" or "license" for more information.
  >>> import re
  >>> def foo(match):
  ...     return "12345"
  ... 
  >>> matcher = re.compile("\s+(aaaaaa)\s+")
  >>> matcher.sub(foo,"                    aaaaaa             ")
  '12345'
  >>> 

If you want to use sub, you have to capture the other groups so you can put the entire match back together (or call replace on group 0, which is equivalent to what i was doing before, just more verbose)

I'm happy to go a little further, but at some point, this code is just not worth the time of using this script.
If you want to spend time to fix all of these issues, and have the energy to take this over and make it what you want, that would be awesome.
I'm just trying to make it work better than it did.



================
Comment at: utils/update_test_checks.py:73
+# spaces, commas, paren, or end of the string
+IR_VALUE_RE = re.compile(r'(\s+)%(.+?)([,\s\(\)]|\Z)')
 
----------------
bryant wrote:
> How come you added more capture groups? Isn't this the syntax that you're expecting?
> 
> FILE ::== LINE*
> LINE ::== LINE_ENT+
> LINE_ENT ::== IR_VALUE | INTERVENING_STUFF
> 
> And the purpose of all this is to capture and rename IR_VALUEs, right?
Because they are needed to make sub work, see below.


================
Comment at: utils/update_test_checks.py:175
 def genericize_check_lines(lines):
+  global vars_seen
   lines_with_def = []
----------------
bryant wrote:
> Is there a reason for moving vars_seen to the top level scope? I don't think that `global` is needed.
> 
> ```
> def generic_check(lines):
>     vars_seen = set()
> 
>     def ssaify(match):
>         v = match.group(1)
>         if v in vars_seen:
>             rv = get_value_use(v)
>         else:
>             vars_seen.add(v) # this works just fine.
>             rv = get_value_definition(v)
>         return the new match which is match.group(1) in the previous IR_VALUE_RE
> ```
> 
> Global would only be needed if you're reassigning the variable vars_seen, whereas the update here is done with a method call.
I'm aware you can do this, i just tend to find nested functions much uglier to follow in simple scripts like this one.
Again, i don't care about this code enough, so if you wanted a nested function, i'll make one.


https://reviews.llvm.org/D28384