Match names more precisely in update_lit_checks.py #6190
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
|
|
||
| ;; CHECK-BIN-NODEBUG: (export "test1" (func $fimport$0)) | ||
|
|
||
| ;; CHECK-BIN-NODEBUG: (export "test2" (global $gimport$0)) |
There was a problem hiding this comment.
Looks less readable here, as it emits it out past the end of the module for some reason?
There was a problem hiding this comment.
This is because round-tripping through the binary does not preserve the names we are now correctly matching against. That's not a new problem with this change, although it now correctly shows up in this case.
There was a problem hiding this comment.
But before it looks like it was just in the right place? I mean, before we had this:
;; CHECK-BIN: (import "env" "test1" (func $test1 (type $v)))
;; CHECK-BIN-NODEBUG: (import "env" "test1" (func $fimport$0 (type $0)))So yeah, we didn't round-trip the name $test1 but still the imports were perfectly aligned?
There was a problem hiding this comment.
It was in a nice place for the wrong reasons. If the input had more imports from "env", then they would all be bunched up together in the output rather than actually matched up with their corresponding input.
There was a problem hiding this comment.
Oh, I actually think grouping all imports that are identical in module and name makes sense - do you disagree? In the quote from my last comment, env and test1 are identical in both lines, so it's nice to keep them together.
I can see how a more strict approach can justify moving them away, but I think it's a loss for readability. Could we perhaps both match names more precisely and also secondarily keep imports/exports with identical external names close together? Then there would not be a regression here from my perspective.
But if that's very difficult to do I won't insist.
There was a problem hiding this comment.
There are other cases (such as in the next PR) where this improves readability by correctly matching imports with their inputs where they would not have otherwise been grouped together.
The update script never reorders the input or the output, so in that sense all the output imports will always be grouped together, but now the way they are interleaved with the input is more consistent and the result is more readable in some cases, but not all cases.
Doing better than making the interleaving consistent would require reordering things, and that's not possible or desirable right now.
Previously the lit test update script interpreted module names as the names of import items and export names as the names of export items, but it is more precise to use the actual identifiers of the imported or exported items as the names instead. Update update_lit_checks.py to use a more correct regex to match names and to correctly use the identifiers of import and export items as their names. In some cases this can improve the readability of test output.
tlively
force-pushed
the
regex-improvements
branch
from
62b668c to
b5a1ed8
Compare
Previously the lit test update script interpreted module names as the names of import items and export names as the names of export items, but it is more precise to use the actual identifiers of the imported or exported items as the names instead. Update update_lit_checks.py to use a more correct regex to match names and to correctly use the identifiers of import and export items as their names. In some cases this can improve the readability of test output.
Previously the lit test update script interpreted module names as the names of
import items and export names as the names of export items, but it is more
precise to use the actual identifiers of the imported or exported items as the
names instead.
Update update_lit_checks.py to use a more correct regex to match names and to
correctly use the identifiers of import and export items as their names. In some
cases this can improve the readability of test output.