capi: find_exported_function #633

gumb0 · 2020-11-03T12:28:48Z

I'm not completely sure it's a good idea to include this, because complications with context can be confusing, but the motivation is

to make it simpler to import exported function into another module - see capi.imported_function_from_anothe_module test change.
symmetry with C++ API fizzy::find_exported_* functions.

axic · 2020-11-03T16:46:53Z

I think this would also require a benchmark to see if the overhead changed.

gumb0 · 2020-11-03T19:20:00Z

I think this would also require a benchmark to see if the overhead changed.

Well the old option to access exported function via index stays, and FizzyCEngine still uses it as before. So what kind of benchmark do you have in mind?

codecov · 2020-11-03T19:53:33Z

Codecov Report

Merging #633 (216909e) into master (c044c54) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #633   +/-   ##
=======================================
  Coverage   98.37%   98.38%           
=======================================
  Files          69       69           
  Lines        9682     9712   +30     
=======================================
+ Hits         9525     9555   +30     
  Misses        157      157

chfast

Seems ok, but there is always a question if this can be done without allocating new object on heap.

include/fizzy/fizzy.h

axic · 2020-11-09T11:11:09Z

include/fizzy/fizzy.h

+/// @param  instance        Pointer to instance.
+/// @param  name            The function name. NULL-terminated string. Cannot be NULL.
+/// @param  out_function    Pointer to output struct to store the found function. Cannot be NULL.
+/// @param  out_context_ptr Pointer to opaque output pointer to a context associated with this


Should also mention that in the case of failure (returns false), what will be the value of this (whether undefined or set to null) and whether that needs to be freed, or not.

Added a sentence.

gumb0 · 2020-11-09T11:13:04Z

Seems ok, but there is always a question if this can be done without allocating new object on heap.

I think only if it were allocated on the heap already on C++ side. Or if we made some other, yet more complicated change in C++ to get rid of std::function.

axic · 2020-11-09T15:11:15Z

include/fizzy/fizzy.h

+///
+/// @note   This function must be called only with external functions returned from
+///         fizzy_find_exported_function.
+void fizzy_free_exported_function(const FizzyExternalFunction* external_function);


I wonder if we should actually make this work on a copy (fizzy_free_exported_function(FizzyExternalFunction)), given the only thing we're freeing is a pointer in the struct.

From an API PoV I think the current one makes more sense, but it may not be fully intuitive for the user?

Not sure, it's a function operating on a struct, so we pass it by const pointer to avoid unnecessaty copying, seems usual for C ?

I mean the fact we don't obtain it as a pointer, but rather allocate it on the stack in the caller.

Yeah I see, I'm not sure what's better.

Another option is to call it with _context anyway: fizzy_free_exported_function_context(FizzyExternalFunction func) - this might be less weird than passing an instance copy to a function that's supposed to "free function".

According to comment #633 (comment) @chfast seems to prefer FizzyExternalFunction* argument

axic · 2020-11-09T15:12:15Z

include/fizzy/fizzy.h

+
+/// Free resources associated with exported function.
+///
+/// If passed pointer is NULL, has no effect.


This is a bit confusing since we never return FizzyExternalFunction as a pointer.

Well we pass it by non-const pointer in fizzy_find_exported_function

I understand that. This comment is connected to the one above.

chfast · 2020-11-11T11:36:23Z

lib/fizzy/capi.cpp

@@ -120,6 +120,20 @@ inline auto unwrap(FizzyExternalFn func, void* context) noexcept
    };
 }

+inline FizzyExternalFunction wrap(fizzy::ExternalFunction external_func)
+{
+    FizzyExternalFn c_function = [](void* context, FizzyInstance* instance, const FizzyValue* args,


Suggested change

FizzyExternalFn c_function = [](void* context, FizzyInstance* instance, const FizzyValue* args,

static constexpr FizzyExternalFn c_function = [](void* context, FizzyInstance* instance, const FizzyValue* args,

chfast · 2020-11-11T11:39:36Z

lib/fizzy/capi.cpp

+        return wrap((func->function)(*unwrap(instance), unwrap(args), depth));
+    };
+
+    auto context = std::make_unique<fizzy::ExternalFunction>(std::move(external_func));


This is not great, but it can stay until we remove usage of std::function.

chfast · 2020-11-11T11:41:47Z

include/fizzy/fizzy.h

+///
+/// @note   This function must be called only with external functions returned from
+///         fizzy_find_exported_function.
+void fizzy_free_exported_function(const FizzyExternalFunction* external_function);


Suggested change

void fizzy_free_exported_function(const FizzyExternalFunction* external_function);

void fizzy_free_exported_function(FizzyExternalFunction* external_function);

Although technically possible, this should be passed without const because you are logically modifying the object.

My motivation was to show exactly that the object itself is not modifed, to make it look a bit less weird when we call it with an instance on stack.

FizzyExternalFunction function; ... fizzy_free_exported_function(&function);

But I don't insist, removed const now.

axic · 2020-11-17T16:49:54Z

What is the status here?

gumb0 · 2020-11-17T17:03:01Z

What is the status here?

It's ready from my side.

You suggested passing argument by value to fizzy_free_exported_function, but I prefer for it to stay a pointer.

axic · 2020-11-17T19:43:49Z

I think neither of the options are good, but it is still better than not having it. Probably with the Rust implementation we'll be informed if we should revisit this.

axic · 2020-11-17T19:54:05Z

test/unittests/capi_test.cpp

+    ASSERT_NE(function.function, nullptr);
+    EXPECT_THAT(function.function(function.context, instance, nullptr, 0), CResult(42));
+
+    fizzy_free_exported_function(&function);


Since you state fizzy_free_exported_function(nullptr) works, can you add it somewhere?

It was a mistake, it's not allowed, will change the comment.

chfast · 2020-11-17T19:54:23Z

include/fizzy/fizzy.h

@@ -275,6 +275,28 @@ uint8_t* fizzy_get_instance_memory_data(FizzyInstance* instance);
 /// @note    Function returns memory size regardless of whether memory is exported or not.
 size_t fizzy_get_instance_memory_size(FizzyInstance* instance);

+/// Find exported function by name.
+///
+/// @param  instance        Pointer to instance.


Suggested change

/// @param instance Pointer to instance.

/// @param instance Pointer to instance. Cannot be NULL?

Added here and in other functions.

gumb0 force-pushed the capi-find-exports branch from 4fcf1d4 to 218da78 Compare November 3, 2020 13:24

This was referenced Nov 3, 2020

Public C API #558

Open

capi: imported and exported tables #628

Merged

gumb0 force-pushed the capi-find-exports branch 4 times, most recently from 5203da1 to d045bf1 Compare November 3, 2020 19:14

gumb0 force-pushed the capi-find-exports branch 5 times, most recently from 4ab7c9d to 9e0a584 Compare November 4, 2020 15:13

gumb0 marked this pull request as ready for review November 4, 2020 15:19

gumb0 force-pushed the capi-find-exports branch 3 times, most recently from f09f1f4 to 0036adc Compare November 4, 2020 17:28

gumb0 requested review from chfast and axic November 4, 2020 17:33

chfast reviewed Nov 5, 2020

View reviewed changes

axic force-pushed the capi-find-exports branch 2 times, most recently from c609c6a to 6849511 Compare November 6, 2020 21:58

axic reviewed Nov 8, 2020

View reviewed changes

include/fizzy/fizzy.h Outdated Show resolved Hide resolved

axic reviewed Nov 8, 2020

View reviewed changes

include/fizzy/fizzy.h Outdated Show resolved Hide resolved

gumb0 force-pushed the capi-find-exports branch from 6849511 to 2f4b021 Compare November 9, 2020 10:58

gumb0 requested a review from axic November 9, 2020 10:59

axic reviewed Nov 9, 2020

View reviewed changes

gumb0 force-pushed the capi-find-exports branch from 2f4b021 to 571ed0b Compare November 9, 2020 11:17

gumb0 marked this pull request as draft November 9, 2020 11:53

gumb0 force-pushed the capi-find-exports branch from be82911 to 97ebaca Compare November 9, 2020 14:08

gumb0 marked this pull request as ready for review November 9, 2020 14:11

gumb0 requested review from axic and chfast November 9, 2020 14:25

axic reviewed Nov 9, 2020

View reviewed changes

chfast reviewed Nov 11, 2020

View reviewed changes

gumb0 force-pushed the capi-find-exports branch from 97ebaca to 3966d37 Compare November 11, 2020 13:56

gumb0 requested review from axic and chfast November 17, 2020 14:31

axic approved these changes Nov 17, 2020

View reviewed changes

axic reviewed Nov 17, 2020

View reviewed changes

chfast approved these changes Nov 17, 2020

View reviewed changes

gumb0 added 3 commits November 18, 2020 11:37

capi: Rename find_exported_function to find_exported_function_index

9a762e2

capi: Add function to find exported function

18a9ca3

capi: Add more comments for requiring non-const argument.

216909e

gumb0 force-pushed the capi-find-exports branch from 3966d37 to 216909e Compare November 18, 2020 10:54

gumb0 requested review from axic and chfast November 18, 2020 10:59

axic approved these changes Nov 18, 2020

View reviewed changes

gumb0 merged commit 0ae57d0 into master Nov 18, 2020

gumb0 deleted the capi-find-exports branch November 18, 2020 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

capi: find_exported_function #633

capi: find_exported_function #633

gumb0 commented Nov 3, 2020 •

edited

Loading

axic commented Nov 3, 2020

gumb0 commented Nov 3, 2020

codecov bot commented Nov 3, 2020 •

edited

Loading

chfast left a comment

axic Nov 9, 2020

gumb0 Nov 9, 2020

gumb0 commented Nov 9, 2020

axic Nov 9, 2020

gumb0 Nov 9, 2020

axic Nov 9, 2020

gumb0 Nov 9, 2020

gumb0 Nov 17, 2020

axic Nov 9, 2020

gumb0 Nov 9, 2020

axic Nov 9, 2020

chfast Nov 11, 2020

chfast Nov 11, 2020

chfast Nov 11, 2020

gumb0 Nov 11, 2020

axic commented Nov 17, 2020

gumb0 commented Nov 17, 2020

axic commented Nov 17, 2020

axic Nov 17, 2020

gumb0 Nov 18, 2020

gumb0 Nov 18, 2020

chfast Nov 17, 2020

gumb0 Nov 18, 2020

	FizzyExternalFn c_function = [](void* context, FizzyInstance* instance, const FizzyValue* args,
	static constexpr FizzyExternalFn c_function = [](void* context, FizzyInstance* instance, const FizzyValue* args,

	void fizzy_free_exported_function(const FizzyExternalFunction* external_function);
	void fizzy_free_exported_function(FizzyExternalFunction* external_function);

	/// @param instance Pointer to instance.
	/// @param instance Pointer to instance. Cannot be NULL?

capi: find_exported_function #633

capi: find_exported_function #633

Conversation

gumb0 commented Nov 3, 2020 • edited Loading

axic commented Nov 3, 2020

gumb0 commented Nov 3, 2020

codecov bot commented Nov 3, 2020 • edited Loading

Codecov Report

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Nov 17, 2020

gumb0 commented Nov 17, 2020

axic commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Nov 3, 2020 •

edited

Loading

codecov bot commented Nov 3, 2020 •

edited

Loading