WasmBinaryReader: Consistent internal naming #6812
Conversation
|
|
||
| ;; CHECK-BIN-NODEBUG: (import "asm2wasm" "f64-rem" (func $fimport$2 (type $4) (param f64 f64) (result f64))) | ||
|
|
||
| ;; CHECK-BIN-NODEBUG: (memory $0 4096 4096) |
There was a problem hiding this comment.
Do you know why these move all the way down to line 1837? That makes things harder to read.
There was a problem hiding this comment.
This is an auto-update, so no idea. @tlively ? Maybe this file was mistakenly manually updated at some point?
There was a problem hiding this comment.
It's because the name of the memory changed from $0 to $m0. The auto update script matches input with output based on name, so when the name changes, the output moves to the bottom with the other unmatched output.
There was a problem hiding this comment.
Ah, that looks wrong, then - if the memory had a name $0, then it should stay $0?
There was a problem hiding this comment.
Sounds like a bug in the text parser maybe?
There was a problem hiding this comment.
Hmm, sorry, I'm still confused - can you connect it back to the original question? I agree that CHECK-BIN-NODEBUG does not preserve the name section, but why does that fact explain this PR's original form (before the workaround) moving line 84 all the way down to 1837?
There was a problem hiding this comment.
I am guessing the renaming here somehow interacts with the auto-updater in a bad way, and my concern is that we can work around it here, but this PR modifies a great many tests, and maybe many of them will need such workarounds.
There was a problem hiding this comment.
Re-reading #6812 (comment) is seems like the auto-updater can only inline stuff when the names match.
In cases like this when we have no names section, I think the only way to make the names match is to explicitly choose the same names that that auto-namer would choose. If you choose any other name then there won't be a match. Is that right @tlively ?
There was a problem hiding this comment.
To make sure I follow, so it worked before because the automatic name was $N, and now to keep that working we need to pick $mN?
|
Suspending work on this until #6813 lands |
For defined functions/globals/tables/memories use `f1`, `t2`, `m3` etc. This matches the longer names used for imports such as `fimport$1`, but keeps the names nice and short by avoiding the embedded dollar sign.
|
I'm curious what you think of the general direction here? Are these names important? Should the be short? Should they be only numbers in some cases or should we always use some kind of type prefix? |
|
I think that short names with disambiguating prefixes sound useful 👍 |
For defined functions/globals/tables/memories use
f1,t2,m3etc.This matches the longer names used for imports such as
fimport$1, but keeps the names nice and short by avoiding the embedded dollar sign.