Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove (attr 0) from tag text format #3946

Merged
merged 1 commit into from
Jun 20, 2021
Merged

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Jun 18, 2021

This attribute is always 0 and reserved for future use. In Binayren's
unofficial text format we were writing this field as (attr 0), but we
have recently come to the conclusion that this is not necessary.

Relevant discussion:
WebAssembly/exception-handling#160 (comment)

This attribute is always 0 and reserved for future use. In Binayren's
unofficial text format we were writing this field as `(attr 0)`, but we
have recently come to the conclusion that this is not necessary.

Relevant discussion:
WebAssembly/exception-handling#160 (comment)
@aheejin aheejin requested a review from tlively June 18, 2021 22:07
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you already have this planned for a follow-up PR, but it would also make sense to remove the attribute field from the Event class for similar reasons.

@aheejin
Copy link
Member Author

aheejin commented Jun 18, 2021

You mean, remove the field altogether and just write 0 in its place instead? That sounds OK, but not very sure it's worth the hassle of deleting the enum and such, in case we add another field to that enum in future. Usually a single module will have just a few (actually, mostly 1) tag anyway, so the memory saving will not be noticeable either.

@tlively
Copy link
Member

tlively commented Jun 18, 2021

Yeah, agreed it probably doesn't make much of a difference. It would be slightly simpler overall to treat the 0 as purely an encoding detail, though.

@aheejin
Copy link
Member Author

aheejin commented Jun 18, 2021

Not very sure what "treat the 0 as purely an encoding detail" means. I thought we were talking about one of these:

// src/wasm.h
enum WasmTagAttribute : uint8_t { WASM_TAG_ATTRIBUTE_EXCEPTION = 0x0 };

class Tag : public Importable {
public:
  uint8_t attribute = WASM_TAG_ATTRIBUTE_EXCEPTION;
  Signature sig;
};

// src/wasm/wasm-binary.cpp
o << uint8_t(tag->attribute);

vs.

// src/wasm.h
class Tag : public Importable {
public:
  Signature sig;
};

// src/wasm/wasm-binary.cpp
o << uint8_t(0);

Not sure why the latter treats 0 as purely an encoding detail while the former does not..?

By the way, another reason I prefer the status quo is it make it clear that what the reserved field is. If we write just 0 we will have to write some comments on what that 0 is. But I admit this is a pretty nitty point though.

@tlively
Copy link
Member

tlively commented Jun 19, 2021

Yes, we’re thinking about the same potential change. By “pure encoding detail” I mean that only the binary parser and binary writer need to know that it exists at all. This is similar to Rossberg’s suggestion that it only be mentioned in the binary format part of the spec.

@aheejin aheejin merged commit 9fc2762 into WebAssembly:main Jun 20, 2021
@aheejin aheejin deleted the remove_attr branch June 20, 2021 06:06
aheejin added a commit to aheejin/binaryen that referenced this pull request Jun 20, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder.

Suggested in
WebAssembly#3946 (review).
aheejin added a commit to aheejin/binaryen that referenced this pull request Jun 20, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder. This also removes
the `attribute` parameter from `makeTag` and `addTag` methods in
wasm-builder.h and binaryen-c.cpp/h.

Suggested in
WebAssembly#3946 (review).
aheejin added a commit to aheejin/binaryen that referenced this pull request Jun 20, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder. This also removes
the `attribute` parameter from `makeTag` and `addTag` methods in
wasm-builder.h, C API, and Binaryen JS API.

Suggested in
WebAssembly#3946 (review).
@aheejin
Copy link
Member Author

aheejin commented Jun 20, 2021

Yes, we’re thinking about the same potential change. By “pure encoding detail” I mean that only the binary parser and binary writer need to know that it exists at all. This is similar to Rossberg’s suggestion that it only be mentioned in the binary format part of the spec.

Done in #3947.

aheejin added a commit to aheejin/binaryen that referenced this pull request Jun 20, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder. This also removes
the `attribute` parameter from `makeTag` and `addTag` methods in
wasm-builder.h, C API, and Binaryen JS API.

Suggested in
WebAssembly#3946 (review).
aheejin added a commit to aheejin/binaryen that referenced this pull request Jun 20, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder. This also removes
the `attribute` parameter from `makeTag` and `addTag` methods in
wasm-builder.h, C API, and Binaryen JS API.

Suggested in
WebAssembly#3946 (review).
aheejin added a commit to aheejin/binaryen that referenced this pull request Jun 20, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder. This also removes
the `attribute` parameter from `makeTag` and `addTag` methods in
wasm-builder.h, C API, and Binaryen JS API.

Suggested in
WebAssembly#3946 (review).
aheejin added a commit that referenced this pull request Jun 22, 2021
This removes `attribute` field from `Tag` class, making the reserved and
unused field known only to binary encoder and decoder. This also removes
the `attribute` parameter from `makeTag` and `addTag` methods in
wasm-builder.h, C API, and Binaryen JS API.

Suggested in
#3946 (review).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants