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

C API function type #557

Merged
merged 1 commit into from
Oct 15, 2020
Merged

C API function type #557

merged 1 commit into from
Oct 15, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Sep 28, 2020

No description provided.

@gumb0 gumb0 force-pushed the capi_func_type branch 3 times, most recently from 5cd07b1 to 9c3ee98 Compare September 28, 2020 14:54
@codecov
Copy link

codecov bot commented Sep 28, 2020

Codecov Report

Merging #557 into master will decrease coverage by 0.00%.
The diff coverage is 96.82%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
- Coverage   98.26%   98.26%   -0.01%     
==========================================
  Files          63       63              
  Lines        9238     9282      +44     
==========================================
+ Hits         9078     9121      +43     
- Misses        160      161       +1     


inline fizzy::FuncType unwrap(const FizzyFunctionType& type)
{
fizzy::FuncType func_type{std::vector<fizzy::ValType>(type.inputs_size),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could optimize it later on the C++ side to not require vector allocation to pass function type (i.e. replace vector with span in fizzy::FuncType)

@@ -108,25 +145,20 @@ void fizzy_free_module(const FizzyModule* module)
delete unwrap(module);
}

FizzyFunctionType fizzy_get_function_type(const FizzyModule* module, uint32_t func_idx)
{
return wrap(unwrap(module)->get_function_type(func_idx));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: get_function_type returns reference to fizzy::FuncType stored in module, then wrap returns pointers to its vector's data.

@gumb0 gumb0 marked this pull request as ready for review October 13, 2020 10:52
@gumb0 gumb0 requested review from chfast and axic October 13, 2020 10:59
@gumb0
Copy link
Collaborator Author

gumb0 commented Oct 13, 2020

static const

The global const means static const in C++, but not in C. This will make them equal.

This will create their copies in each translation unit that includes fizzy.h, is this what we want?

@chfast
Copy link
Collaborator

chfast commented Oct 13, 2020

This will create their copies in each translation unit that includes fizzy.h, is this what we want?

They are going to be used as compile-time constants, unlikely to be emitted at all.

@axic
Copy link
Member

axic commented Oct 14, 2020

@gumb0 can you please rebase and squash?

@gumb0
Copy link
Collaborator Author

gumb0 commented Oct 14, 2020

@gumb0 can you please rebase and squash?

Done, squashed the last commit.

/// Input types array size.
size_t inputs_size;
/// Pointer to output types array.
const FizzyValueType* outputs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is ok, but also can be greatly simplified by defining FizzyValueTypeVoid and using FizzyValueType output_type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I'll try it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Move it to the front.

static const FizzyValueType FizzyValueTypeF32 = 0x7d;
static const FizzyValueType FizzyValueTypeF64 = 0x7c;
/// Special value, can be used only as function outuput type.
static const FizzyValueType FizzyValueTypeVoid = 0;
Copy link
Collaborator Author

@gumb0 gumb0 Oct 14, 2020

Choose a reason for hiding this comment

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

Defining it as 0 allows user to create void type with no params as {}, at least in C++, probably in C, too.

/// Function type.
typedef struct FizzyFunctionType
{
/// Output type, can be FizzyValueTypeVoid, if function has no output.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Output type, can be FizzyValueTypeVoid, if function has no output.
/// Output type, equals to FizzyValueTypeVoid, if function has no output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

/// defined in module.
///
/// @note All module function indices are greater than all imported function indices.
FizzyFunctionType fizzy_get_function_type(const FizzyModule* module, uint32_t func_idx);
Copy link
Member

Choose a reason for hiding this comment

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

What happens on invalid index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a sentence.

@axic
Copy link
Member

axic commented Oct 14, 2020

Needs rebase.

@gumb0
Copy link
Collaborator Author

gumb0 commented Oct 14, 2020

Rebased.

@gumb0 gumb0 force-pushed the capi_func_type branch 2 times, most recently from 95246c5 to 9991225 Compare October 14, 2020 16:46
///
/// @param module Pointer to module.
/// @param func_idx Function index. Can be either index of an imported function or of a function
/// defined in module. Behaviour is undefined, index is not valid according to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// defined in module. Behaviour is undefined, index is not valid according to
/// defined in module. Behaviour is undefined if index is not valid according to

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Looks okayish to me.

0.6.0 automation moved this from In progress to Reviewer approved Oct 14, 2020
@gumb0 gumb0 merged commit f500580 into master Oct 15, 2020
0.6.0 automation moved this from Reviewer approved to Done Oct 15, 2020
@gumb0 gumb0 deleted the capi_func_type branch October 15, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants