Properly stringify names in tests #6279
Conversation
| Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| ;; CHECK: (type $0 (func)) | ||
| | ||
| ;; CHECK: (type $[i8] (array i8)) | ||
| (type $[i8] (array i8)) |
There was a problem hiding this comment.
This moved away because the names no longer match, I guess? Should we update both at once?
There was a problem hiding this comment.
Yeah, and it looks like the follow-up hasn't fixed it fully. Updating both at once makes sense.
There was a problem hiding this comment.
@kripken PTAL. The commits on this PR should be separately reviewable, but we can land them all together to avoid intermediate test regressions.
tlively changed the title tlively changed the title src/support/string.h Outdated
| } | ||
| } | ||
| return os << '"'; | ||
| } |
There was a problem hiding this comment.
Minor nit, the changes here and in name.h are not performance-sensitive (printing text isn't) and so they could be in the .cpp files instead, for faster compile times.
There was a problem hiding this comment.
Sure, I'll add a new .cpp files for these.
There was a problem hiding this comment.
Github thinks this is a binary file... any idea why that is?
It doesn't seem to have thought it was binary before: https://github.com/WebAssembly/binaryen/blob/main/test/spec/comments.wast
There was a problem hiding this comment.
Hmm, maybe my update script broke some unicode in this file. I'll revert it.
test/hello_world.wat Outdated
| @@ -1,5 +1,5 @@ | |||
| (module | |||
| (type $i32_i32_=>_i32 (func (param i32 i32) (result i32))) | |||
| (type $$i32_i32_=>_i32 (func (param i32 i32) (result i32))) | |||
There was a problem hiding this comment.
Hmm, not sure. Reverted it locally and re-updated the tests and it never came back.
tlively force-pushed the parser-string-names branch from 77ff8b6 to 00a5702 Compare 
Update identifiers used in tests to use a format supported by the new text parser, i.e. either the standard format with its limited set of allowed characters or the non-standard `$"..."` format. Notably, any name containing square or curly braces now uses the string format. Input automatically updated with this script: https://gist.github.com/tlively/4e22311736661849e641d02e521a0748 Some test output that the script updates is reverted by the test updater script because the printer does not escape all the names it should. This will be addressed in a follow-up PR.
There were a few places in Print.cpp where we were printing names directly (e.g. `o << '$' << name`) without proper escaping. Fix those places and update the test output.
The next commit will use the newly shared name printing logic to correctly escape type names when printing types.
tlively force-pushed the test-input-names branch from 9a18852 to c0b52e0 Compare 
Update identifiers used in tests to use a format supported by the new text parser, i.e. either the standard format with its limited set of allowed characters or the non-standard `$"..."` format. Notably, any name containing square or curly braces now uses the string format. Input automatically updated with this script: https://gist.github.com/tlively/4e22311736661849e641d02e521a0748 The printer is updated to properly escape names in more places as well. The logic for escaping names is moved to a common location so that the type printing logic in wasm-type.cpp can use it as well.
2 participants
Update identifiers used in tests to use a format supported by the new text
parser, i.e. either the standard format with its limited set of allowed
characters or the non-standard
$"..."format. Notably, any name containingsquare or curly braces now uses the string format.
Input automatically updated with this script:
https://gist.github.com/tlively/4e22311736661849e641d02e521a0748
The printer is updated to properly escape names in more places as well. The
logic for escaping names is moved to a common location so that the type
printing logic in wasm-type.cpp can use it as well.