Skip to content

Conversation

@arthurschreiber
Copy link

No description provided.

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.

I like the direction of this Pull Request and the idea in general! Let's make this happen 😁 Thank you for your effort!

src/frontend.ts Outdated

let result = [
new this.implementation.node.Int(
new frontend.node.Int(this.id.id(name), field, bytes, signed, littleEndian, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Let's split it into multiple statements. Something like: const tmp = new frontend.node.Int(...).

src/frontend.ts Outdated
let next = result[0];
for (let offset = 1; offset < node.bytes; offset++) {
result[offset] = new this.implementation.node.Int(
new frontend.node.Int(this.id.id(name + '_byte' + (offset + 1)), field, bytes, signed, littleEndian, offset)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: similar recommendation here. Perhaps even move this.id.id() to a separate statement too. Also, would be great opportunity to use template syntax here: ${name}_byte${offset + 1}.

@indutny
Copy link
Member

indutny commented Jan 6, 2020

Oh and of course it needs a test!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants