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

src: pack struct by sorting fields #36

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

src: pack struct by sorting fields #36

wants to merge 2 commits into from

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Jan 11, 2020 •
edited
Loading

Issue #35
Decide just drop code, interesting thing.

Fields in struct same, but by some cases errors on tests. Currently do not know llparse too good for say why 🤔

btw, should not i8/i16/i32/i64 be u8/u16/u32/u64?

@fanatid fanatid mentioned this pull request Jan 11, 2020
Closed
indutny
indutny requested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM in general, but it has to cover bitcode as well! C headers apply both to C and bitcode output.

@fanatid
Copy link
Contributor Author

fanatid commented Jan 14, 2020

Sorry, I do not think that I understand. I should change something in bitcode for fixing tests?

@indutny
Copy link
Member

indutny commented Jan 14, 2020

Not in the tests. The order of fields has to be changed in the bitcode generator.

@fanatid
Copy link
Contributor Author

fanatid commented Jan 14, 2020

Thank you.
I see that I also need change fields in bitcode / js:

llparse/src/implementation/bitcode/compilation.ts

Line 124 in 564e5c4

this.state.addField(constants.TYPE_INDEX, constants.STATE_INDEX);

llparse/src/implementation/js/index.ts

Line 47 in 564e5c4

out.push(` ${ctx.indexField()} = 0;`);

@indutny
Copy link
Member

indutny commented Jan 15, 2020

Well, maybe not in JS, but you got it right!

@fanatid
Copy link
Contributor Author

fanatid commented Jan 20, 2020

I fixed code for bitcode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indutny indutny indutny requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@fanatid @indutny