Drop support for non-standard quoted function names #6188
Conversation
We previously supported a non-standard `(func "name" ...` syntax for declaring functions exported with the quoted name. Since that is not part of the standard text format, drop support for it, replacing it with the standard `(func $name (export "name") ...` syntax instead. Also replace our other usage of the quoted form in our text output, which was where we quoted names containing characters that are not allowed to appear in standard names. To handle that case, adjust our output from `"$name"` to `$"name"`, which is the standards-track way of supporting such names. Also fix how we detect non-standard name characters to match the spec. Update the lit test output generation script to account for these changes, including by making the `$` prefix on names mandatory. This causes the script to stop interpreting declarative element segments with the `(elem declare ...` syntax as being named "declare", so prevent our generated output from regressing by printing a real name for these segments to ensure the output generation script continues including them.
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
| @@ -4,12 +4,12 @@ | |||
| (type $0 (func (param f32 i31ref i64 f64 funcref))) | |||
| (type $1 (func (result funcref))) | |||
| (global $global$0 (mut funcref) (ref.null nofunc)) | |||
| (elem declare func $0) | |||
| (export "export" (func $1)) | |||
| (elem $decl declare func$0) | |||
There was a problem hiding this comment.
Oh, this is the last paragraph in the description I guess. But why does fixing the parsing force us to add names here? It is valid even without the name, isn't it? If so doesn't that mean we no longer parse valid code?
There was a problem hiding this comment.
This is an output .txt file, so the printer now names this. We don't need to name declarative segments in the input.
There was a problem hiding this comment.
Can we avoid printing this name in the output, if it's unnecessary? It just seems to add clutter compared to before.
There was a problem hiding this comment.
The auto update script only recognizes named items, so even with --all-items, it won't include these declarative segments in the output unless we give them names. It previously included them because it thought "declare" was its name.
Are you ok with dropping declarative element segments from generated test output? If so, I can remove this emitted name. Otherwise we can consider trying to change how the update script works, but that is very difficult.
There was a problem hiding this comment.
I'm confused, didn't that work before this PR? We didn't have named declarative segments before, so it seems like it must have?
|
I would believe that this might have been a thing pre-1.0, but I don't know for sure. It's definitely not in the spec now. |
|
By counting |
| ;; NORMAL-NEXT: [trap unreachable] | ||
| ;; TNH: [fuzz-exec] calling foo | ||
| ;; TNH-NEXT: [LoggingExternalInterface logging 42] | ||
| ;; TNH-NEXT: [trap unreachable] |
There was a problem hiding this comment.
These lines seem to duplicate new lines 64-67? Might be a pre-existing problem before this PR, but it's odd this PR just hoists one of them and not the other.
There was a problem hiding this comment.
Do you know why the execution log is duplicated in the first place? I think only the first repetition was hoisted because the update script only knows to pair a single ouput item with a given input item.
There was a problem hiding this comment.
No idea. But from what you say the updater script is doing the right thing, so it's definitely a separate issue.
We previously supported a non-standard `(func "name" ...` syntax for declaring functions exported with the quoted name. Since that is not part of the standard text format, drop support for it, replacing it with the standard `(func $name (export "name") ...` syntax instead. Also replace our other usage of the quoted form in our text output, which was where we quoted names containing characters that are not allowed to appear in standard names. To handle that case, adjust our output from `"$name"` to `$"name"`, which is the standards-track way of supporting such names. Also fix how we detect non-standard name characters to match the spec. Update the lit test output generation script to account for these changes, including by making the `$` prefix on names mandatory. This causes the script to stop interpreting declarative element segments with the `(elem declare ...` syntax as being named "declare", so prevent our generated output from regressing by counting "declare" as a name in the script.
We previously supported a non-standard
(func "name" ...syntax for declaringfunctions exported with the quoted name. Since that is not part of the standard
text format, drop support for it, replacing it with the standard
(func $name (export "name") ...syntax instead.Also replace our other usage of the quoted form in our text output, which was
where we quoted names containing characters that are not allowed to appear in
standard names. To handle that case, adjust our output from
"$name"to$"name", which is the standards-track way of supporting such names. Also fixhow we detect non-standard name characters to match the spec.
Update the lit test output generation script to account for these changes,
including by making the
$prefix on names mandatory. This causes the script tostop interpreting declarative element segments with the
(elem declare ...syntax as being named "declare", so prevent our generated output from regressing
by counting "declare" as a name in the script.