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.

Sign up for GitHub

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

Jump to bottom

capi: resolve imported globals #660

Merged
merged 2 commits into from
Jan 28, 2021
Merged

capi: resolve imported globals #660

merged 2 commits into from
Jan 28, 2021

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Nov 23, 2020 •
edited by axic
Loading

Depends on #637.

@codecov
Copy link

codecov bot commented Nov 23, 2020 •
edited
Loading

Codecov Report

Merging #660 (a030f51) into master (c38917f) will decrease coverage by 0.00%.
The diff coverage is 98.76%.

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
- Coverage   99.31%   99.31%   -0.01%     
==========================================
  Files          72       72              
  Lines       10541    10619      +78     
==========================================
+ Hits        10469    10546      +77     
- Misses         72       73       +1
Flag Coverage Δ
spectests 90.84% <ø> (ø)
unittests 99.31% <98.76%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/unittests/capi_test.cpp 99.87% <98.50%> (-0.13%) ⬇️
lib/fizzy/capi.cpp 98.91% <100.00%> (+0.04%) ⬆️

@gumb0 gumb0 force-pushed the capi-resolve-globals branch from 222bf52 to cc64cc2 Compare November 23, 2020 18:35
@gumb0 gumb0 force-pushed the capi-resolve-globals branch from cc64cc2 to 05c36be Compare November 24, 2020 12:01
@gumb0 gumb0 marked this pull request as ready for review November 24, 2020 12:05
chfast
chfast reviewed Nov 24, 2020
View reviewed changes
lib/fizzy/capi.cpp Outdated
{
fizzy::ImportedGlobal imported_global;
imported_global.module =
c_imported_global.module ? std::string{c_imported_global.module} : std::string{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

The c_imported_global.module cannot be null.

@gumb0 gumb0 force-pushed the capi-resolve-globals branch from 05c36be to 5838d7f Compare November 24, 2020 13:42
@gumb0 gumb0 requested review from chfast and axic November 24, 2020 14:44
@gumb0 gumb0 mentioned this pull request Nov 24, 2020
Open
49 tasks
@axic
Copy link
Member

axic commented Dec 21, 2020

Is this a replacement of #637?

@gumb0
Copy link
Collaborator Author

gumb0 commented Dec 21, 2020

Is this a replacement of #637?

#637 is C++ part, and this one is C part on top of it.

@gumb0 gumb0 force-pushed the resolve-imports branch 3 times, most recently from 29b4829 to 14e1a39 Compare January 5, 2021 15:26
@gumb0 gumb0 force-pushed the capi-resolve-globals branch 2 times, most recently from b2f422a to e736eab Compare January 5, 2021 18:14
@gumb0 gumb0 force-pushed the resolve-imports branch 2 times, most recently from 73938d1 to 570e8ef Compare January 27, 2021 11:44
Base automatically changed from resolve-imports to master January 27, 2021 12:17
@gumb0 gumb0 force-pushed the capi-resolve-globals branch 2 times, most recently from ebaebdb to 00aeaa4 Compare January 27, 2021 12:23
axic
axic reviewed Jan 27, 2021
View reviewed changes
include/fizzy/fizzy.h
/// No validation is done on the number of globals passed in, nor on their order.
/// When number of passed globals or their order is different from the one defined by the
/// module, behaviour is undefined.
/// Globals in @a imported_globals are allowed to be in any order and allowed to include some
Copy link
Member

Choose a reason for hiding this comment

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

Are we using #imported_globals now? Or not for arguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not for arguments, I think I tried it already #681 (comment)

axic
axic reviewed Jan 27, 2021
View reviewed changes
include/fizzy/fizzy.h
/// No validation is done on the number of globals passed in, nor on their order.
/// When number of passed globals or their order is different from the one defined by the
/// module, behaviour is undefined.
/// Globals in @a imported_globals are allowed to be in any order and allowed to include some
Copy link
Member

Choose a reason for hiding this comment

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

Does the of lifetime module/name needs to be only valid during fizzy_resolve_instantiate?

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 note.

axic
axic reviewed Jan 27, 2021
View reviewed changes
test/unittests/capi_test.cpp Outdated
FizzyValue result_f32;
result_f32.f32 = 42;
Copy link
Member

Choose a reason for hiding this comment

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

Ugh.

@gumb0 gumb0 force-pushed the capi-resolve-globals branch 2 times, most recently from 7401844 to 436a6cf Compare January 27, 2021 15:15
This was referenced Jan 27, 2021
Merged
Merged
@gumb0 gumb0 force-pushed the capi-resolve-globals branch from e57cf00 to 1d6067a Compare January 27, 2021 17:50
@gumb0 gumb0 force-pushed the capi-resolve-globals branch from 1d6067a to 45e5422 Compare January 27, 2021 18:28
@axic
Copy link
Member

axic commented Jan 27, 2021

Wasn't the last commit merged already?

@gumb0
Copy link
Collaborator Author

gumb0 commented Jan 27, 2021

Wasn't the last commit merged already?

It's base was this PR, so you merged it here.
(it requires FizzyImportedGlobal)

@axic
Copy link
Member

axic commented Jan 27, 2021

Oh, sorry for that!

@axic axic mentioned this pull request Jan 27, 2021
Merged
@axic
Copy link
Member

axic commented Jan 27, 2021

Recreated it in #701.

@axic axic force-pushed the capi-resolve-globals branch from 45e5422 to a030f51 Compare January 27, 2021 19:41
@gumb0 gumb0 merged commit 3ecf546 into master Jan 28, 2021
@gumb0 gumb0 deleted the capi-resolve-globals branch January 28, 2021 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic approved these changes

@chfast chfast Awaiting requested review from chfast

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @axic @chfast