write invalid names using @name annotation
#2297
Conversation
There was a problem hiding this comment.
Thank you! Looks pretty straightforward. Could you please also add a few more tests, e.g. a roundtrip test for the example given in https://github.com/WebAssembly/annotations/blob/main/proposals/annotations/Overview.md (under "Expressing names")?
| @@ -608,7 +608,7 @@ TokenType WastParser::Peek(size_t n) { | |||
| } | |||
| if ((options_->features.code_metadata_enabled() && | |||
There was a problem hiding this comment.
Seems like maybe there's enough conditions here that they would benefit by getting broken out into a separate function at this point?
There was a problem hiding this comment.
Agreed. Honestly, I was a bit surprised to see this logic in the Peek method, too. Perhaps it would be better to put this all in it's own method anyway, with a more straightforward way of parsing/skipping certain annotations.
| @@ -0,0 +1,13 @@ | |||
| ;;; TOOL: run-roundtrip | |||
| ;;; ARGS: --stdout --no-check --enable-annotations --debug-names | |||
There was a problem hiding this comment.
Why is --no-check used here -- what makes the module invalid?
There was a problem hiding this comment.
Ah, I think I must've left this in when looking at another roundtrip test for reference. Thanks for the catch!
| (type (;0;) (func (result i32))) | ||
| (func (@name "$hi hello hey") (type 0) (result i32) | ||
| i32.const 42) | ||
| (@custom "name" "\01\0f\01\00\0chi hello hey\02\03\01\00\00")) |
There was a problem hiding this comment.
Do we want to be printing both the @name annotation and also the @custom "name" annotation with the same info? (Not 100% sure what the right behavior is, but we do suppress the @custom annotation for most other cases that are already handled elsewhere...)
There was a problem hiding this comment.
Hmm, this is the current behavior of names in general. Even without the @name annotation, both are included. I'll work on suppressing this, though.
Looking at this test, it seems I misinterpreted the use of |
|
Upon working on the implementation of the work in my above comment, it seems like there are many possible places one could use the edit: |
|
You raise some good questions but I definitely don't have a view on what the behavior is supposed to be. I guess we should just match other implementations (if there are other implementations -- maybe this is too soon?). It's a little unfortunate that these annotation specifications aren't going to get tests. :-( |
|
I'll wait until WebAssembly/annotations#21 is resolved. If the |
Following up #2284, a few commits were reverted related to the
@nameannotation so that the PR would focus on a single addition. This PR makes use of the@nameannotation when writing names containing invalid characters in the text format (see WebAssembly/spec#617).Let me know if there's anything else I should add!