[PATCH] D70660: Add initial tests for update_{llc,cc}_test_checks.py

Mon Dec 2 10:34:22 PST 2019

arichardson marked an inline comment as done.
arichardson added a comment.

In D70660#1765478 <https://reviews.llvm.org/D70660#1765478>, @jdoerfert wrote:

> I have two minor comments, neither is blocking. @MaskRay @xbolva00, please review and accept without waiting for me, I'm fine with this.
>
> - We still have the "inputs" folder in which everything lives. I could see the '.ll' and '.ll.expected' to live in their but why the tests?

Only the .ll and .ll.expected is in the Inputs folder. The .test shouldn't be.
I could also move the .expected out of the inputs folder which would make some of the check lines simpler since I can just use `diff -u %t.ll %s.expected` then.

> - The `--function-signatures` test has manual check lines. We should probably move to a '.expected' file as well here.

I am happy to move to a separate .expected file for the `--function-signatures test`. What do you think @MaskRay @xbolva00 ?

================
Comment at: llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test:44
+# FUNCSIG-NEXT:  +; CHECK-SAME: (<2 x i8> {{\[}}[X:%.*]], <2 x i8> {{\[}}[Y:%.*]])
+# FUNCSIG-NEXT:   ; CHECK-NEXT:    ret <2 x i8> zeroinitializer
----------------
jdoerfert wrote:
> Shouldn't we have multiple .expected files instead? I see myself struggle with the escaping already.
Since these files are quite small I guess that would be fine. I agree that the escaping is ugly.

The only benefit of having the manual check lines is that it shows the difference between the two files but that can also be seen by comparing the `<test>.expected` with a `<test>.funcsig.expected`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70660