Skip to content

[C-/JS-Api] Avoid add imports for existing entities in BinaryenSetMemoryImport and etc #4991

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

Merged
merged 8 commits into from
Aug 30, 2022

Conversation

MaxGraey
Copy link
Contributor

@MaxGraey MaxGraey commented Aug 30, 2022

Fix #4988.

BinaryenAddMemoryImport
BinaryenAddTableImport
BinaryenGlobalTableImport
BinaryenAddTagImport

Also use unique pointers for entity creation.

Now carefully check if entry already exists and just modify module and base if this true.

@MaxGraey
Copy link
Contributor Author

@tlively @kripken could you take a look on this please?

@MaxGraey MaxGraey changed the title [C-/JS-Api] Add new BinaryenSetMemoryImport method [C-/JS-Api] Avoid add imports for existing entities in BinaryenSetMemoryImport and etc Aug 30, 2022
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm, but please add comments in the .h file to mention that these support both cases now, maybe like this:

diff --git a/src/binaryen-c.h b/src/binaryen-c.h
index a21c0966a..6a9110849 100644
--- a/src/binaryen-c.h
+++ b/src/binaryen-c.h
@@ -2183,7 +2183,9 @@ BINARYEN_API BinaryenIndex BinaryenGetNumFunctions(BinaryenModuleRef module);
 BINARYEN_API BinaryenFunctionRef
 BinaryenGetFunctionByIndex(BinaryenModuleRef module, BinaryenIndex index);
 
-// Imports
+// Imports. These either create a new entity (function/table/memory/etc.) and
+// mark it as an import, or, if an entity already exists with internalName then
+// the existing entity is turned into an import.
 
 BINARYEN_API void BinaryenAddFunctionImport(BinaryenModuleRef module,
                                             const char* internalName,

@MaxGraey
Copy link
Contributor Author

@kripken It's ready for merging

@kripken kripken merged commit da24ef6 into WebAssembly:main Aug 30, 2022
@MaxGraey MaxGraey deleted the fix-mem-import-api branch August 30, 2022 20:02
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.

Import memory caused to Fatal: Module::addMemory: 0 already exists
2 participants