Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

[module] Finalize definitions for the end-user #1290

Merged
merged 1 commit into from
Dec 16, 2019

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Dec 14, 2019

This removes the need for end-users of cranelift-module to call module.finalize_definitions before module.finish is called. It is fine for finalize_definitions to remain pub because the implementation calls .drain() on all relevant data, so it will never be finalized twice.

  • This PR contains test cases, if meaningful.

N/A, no new features.

  • A reviewer from the core maintainer team has been assigned for this PR.
    If you don't know who could review this, please indicate so and/or ping
    bnjbvr. The list of suggested reviewers on the right can help you.

I am not sure who should review this. r? @bnjbvr

Closes bytecodealliance#1288 by
calling `module.finalize_definitions` whenever `module.finish` is
called.
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

It seems fine to me, and from my reading of the issue, to @philipc too, so I'll approve (and let some time for @philipc / @bjorn3 to chime in, if they have a different opinion). Thanks!

Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

There is no reason to allow calling finish without calling finalize_definitions.

Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

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

LGTM

@bnjbvr
Copy link
Member

bnjbvr commented Dec 16, 2019

Thanks! Merging then.

@bnjbvr bnjbvr merged commit 2d3252c into bytecodealliance:master Dec 16, 2019
@jyn514 jyn514 deleted the module-finalize branch December 16, 2019 12:04
@iximeow
Copy link
Collaborator

iximeow commented Jan 16, 2020

just an appreciation comment: I tripped across finalize_definitions not being called when first working out #902, and found that my use of #902 had become incorrect because cranelift is nicer by default :) thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants