[PATCH] D80584: [utils] change update_test_checks.py use of 'TMP' value names

Mon Jun 1 09:07:25 PDT 2020

spatel closed this revision.
spatel added a comment.

In D80584#2066319 <https://reviews.llvm.org/D80584#2066319>, @jdoerfert wrote:

> First, I think the warning is strictly a good thing. Thanks for keeping that in!

Sure - we've been missing that for a long time.

> I don't think the options are really "complexity to the user" given a sensible default (= the old value). If a user sees a warning that says: `Name conflict, use --nameless-prefix=XXXXX, where XXXXX is a string not otherwise used in your IR`, they will happily do so. The UTC_ARGS extension makes it permanent and no one needs to worry about this until they see a warning again. (If we can "hide" the option from the help message, I'm fine with that too.)

I'm still not convinced that's worth doing. I did change instnamer though:
rGdd54432a0f5a <https://reviews.llvm.org/rGdd54432a0f5a6f042fa4d2db3094c6f02e5ad275>
...so between the warning and at least 1 of these not using 'tmp', I think we're good enough.

> Adding a test is just something we should do as it is easy to accidentally break functionality like the warning.

Yep, I didn't remember that we had tests for scripts. So we have that now, and there's a clang test that wiggles on this too...although that seems like we should generalize that file to avoid killing all of the bots like I did. :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80584