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

Add support for remapstart #107

Merged
merged 1 commit into from
Jun 15, 2019
Merged

Add support for remapstart #107

merged 1 commit into from
Jun 15, 2019

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Jun 4, 2019

Fixes #99

libchisel/Cargo.toml Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
axic
axic previously requested changes Jun 5, 2019
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.

As a way to organise I'd say it should:

  1. check if a start section is present. If not, just quit.
  2. check if an export named main exists. If yes, start (recursively) rename so that there is no main function.
  3. insert a new export called main which points to the same function index what the start section was using
  4. remove the start section

@jwasinger
Copy link
Contributor Author

jwasinger commented Jun 5, 2019

check if a start section is present. If not, just quit.

Already implemented as described.

check if an export named main exists. If yes, start (recursively) rename so that there is no main function.

I have a comment in the code because I wasn't sure whether to expect the module to be validly formed when remapstart is run. But I see now that I will have to make a change to fail without panicking when main export is not found.

Also, I'm not sure that implementing this with recursion is much cleaner than what I am currently doing?

insert a new export called main which points to the same function index what the start section was using

Already implemented as described.

remove the start section

Already implemented as described.

libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Show resolved Hide resolved
@axic
Copy link
Member

axic commented Jun 6, 2019

Lets simplify my last comment #107 (review):

As a way to organise I'd say it should:

  1. check if a start section is present. If not, just quit.
  2. drop the names section (not sure if exports are duplicated in names, but lets just drop it and we can fix later)
  3. check if an export named main exists. If yes, remove that entry from the export section.
  4. insert a new export called main which points to the same function index what the start section was using
  5. remove the start section

libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented Jun 6, 2019

@jwasinger can you squash this to a single commit?

@jwasinger jwasinger force-pushed the remapstart branch 2 times, most recently from 4bce354 to 9a9a94d Compare June 6, 2019 19:47
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
libchisel/src/remapstart.rs Outdated Show resolved Hide resolved
@jakelang jakelang force-pushed the remapstart branch 2 times, most recently from d829efb to 2709bfe Compare June 8, 2019 09:51
@jakelang jakelang requested a review from axic June 8, 2019 09:52
@jakelang jakelang dismissed axic’s stale review June 8, 2019 11:16

Addressed review

@jakelang jakelang added ready Ready for review and merge and removed wip labels Jun 13, 2019
@jakelang jakelang force-pushed the remapstart branch 3 times, most recently from 4755482 to 806fd2e Compare June 13, 2019 23:16
@axic axic merged commit d176933 into wasmx:master Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready Ready for review and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translator for start function (remapstart)
3 participants