<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 6, 2017 at 10:38 AM, bryant via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">bryant added inline comments.<br>
<br>
<br>
================<br>
Comment at: utils/update_test_checks.py:<wbr>176<br>
</span><span class="">+      if match.group(1) in vars_seen:<br>
+        line = line.replace('%' + match.group(1), get_value_use(match.group(1)), 1)<br>
+        continue<br>
</span>----------------<br>
<span class="">bryant wrote:<br>
> bryant wrote:<br>
> > You're limiting the number of replaced uses to 1. Can a line have more than one use?<br>
> Just to clarify, what I meant is that you can do a lot better than manually replacing one at a time:<br>
><br>
> ```<br>
> def genericize_check_lines(lines):<br>
>     def ssaify(match):<br>
>         v = match.group(1)<br>
>         if v in var_seen:<br>
>             rv = get_value_use(v)<br>
>         else:<br>
>             vars_seen.add('%' + v)<br>
>             rv = get_value_definition(v)<br>
>         return rv<br>
>     for line in lines:<br>
>         scrubbed_line = ...<br>
>         new_ine = IRVALUE_RE.sub(ssaify, scrubbed_line)<br>
>     ...<br>
> ```<br>
><br>
> I suspect that underneath, `_sre` is building the replacement string incrementally, so it won't be quadratic.<br>
</span><span class="">> Yes, but you want to replace one at a time anyway.<br>
><br>
><br>
> because it could be a use of "bb", and the line could be phi [a, bb], [b, bb17], [c, bb].<br>
><br>
> If you replace  all uses of bb, you'll mistakenly replace bb17.<br>
><br>
</span><span class="">> We can do better, but not in this way (see my other email, this will generate wrong answers) , and it's IMHO not worth it.<br>
><br>
> You can avoid doing the replacement directly by using the match start and end, but if you do, we either have to mess with the loop quite a bit.<br>
> Even if we stop using finditer (because we are replacing the string it is using as it goes, and it does not keep up to date) and use match directly, we have to offset where it starts looking for matches by the amount of text we've added by converting to var defs/uses, etc, or risk false matches.<br>
><br>
><br>
> None of this is worth it compared to keeping it simple like I did, imho.<br>
<br></span></blockquote><div><br></div><div>Given how often we update testcases using it, still don't care.</div><div>But it seems to bug you, so i'll update the patch:)</div><div> </div></div></div></div>