Skip to content

Support (module definition ...) constructs - #2829

Open
zherczeg wants to merge 1 commit into
WebAssembly:mainfrom
zherczeg:definition
Open

Support (module definition ...) constructs#2829
zherczeg wants to merge 1 commit into
WebAssembly:mainfrom
zherczeg:definition

Conversation

@zherczeg

Copy link
Copy Markdown
Collaborator

Related patch: #2666

@sbc100

sbc100 commented Aug 19, 2026

Copy link
Copy Markdown
Member

Does this replace #2666? Is it an alternative implementation?

Comment thread include/wabt/ir.h Outdated
class TextScriptModule : public ScriptModuleMixin<ScriptModuleType::Text> {
public:
const Location& location() const override { return module.loc; }
bool IsDefinition() const override { return is_definition; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason to just just make is_definition a public field on ScriptModule? i.e. do we need to use a virtual method here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No. I will change this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The code is even simpler this way.

@sbc100 sbc100 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! A surprisingly small patch to make this work.

@zherczeg

Copy link
Copy Markdown
Collaborator Author

Yes this is a replacement.

@@ -1,4 +1,4 @@
/* C++ code produced by gperf version 3.2.1 */
/* C++ code produced by gperf version 3.3 */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we do not pin the exact version that we use here?

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.

2 participants