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

[Don't merge] TypedIdentifier, Box<str> and other tweaks #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maciejhirsz
Copy link

@maciejhirsz maciejhirsz commented Nov 6, 2018

Will be crossing out things in this PR as they are moved to their own smaller PRs:

  • Added TypedIdentifier, following the grammar spec, that must have a type. Function definitions and variable declarations now use Vec<TypedIdentifier> instead of Vec<Identifier>.
  • Identifier is now a newtype wrapping a Box<str>, following the grammar spec it does not allow a type to be attached.
    • Using Box<str> instead of String to reduce the size of the AST in memory. Previously Identifier used to take 7 words (56 bytes on 64bit arch), now Identifier takes 2 words while TypedIdentifier takes 5.
  • Added From impls to easily create instances of Identifier from string literals ("foo".into() or Identifier::from("foo")).
  • Literal now uses Box<str> instead of String (same reason as above) and must have a type as per grammar definition.
  • Added convenience From imps for Literal that allows to quickly create them from Rust literals (eg. Literal::from(42u8) == Literal { literal: "42".into(), yultype: Type::Uint8 }).
  • Validator impl for Identifier checks that the identifier is in accordance with the grammar.
  • Changed the error type from String to &'static str in the validator, although custom enum or using the error_chain crate would be better here I reckon.
  • Some Display impls got their list printing reworked with a helper function that is much more efficient. I'm not sure if using Display is the best choice here, a custom trait could be way more efficient and would allow for different formatting options, if needed.

@axic
Copy link
Owner

axic commented Nov 8, 2018

Thank you, this looks great!

Would it be possible to split up the changes into multiple PRs? If so, can we start with one, get it merged and so forth?

@maciejhirsz
Copy link
Author

@axic sure, I'll break down the PR tomorrow.

@axic
Copy link
Owner

axic commented Nov 8, 2018

Nice! Lets just start with a single change first and then we can keep coming back to this PR to pick the other changes from.

@maciejhirsz maciejhirsz mentioned this pull request Nov 12, 2018
@maciejhirsz maciejhirsz changed the title TypedIdentifier, Box<str> and other tweaks [Don't merge] TypedIdentifier, Box<str> and other tweaks Nov 12, 2018
chriseth pushed a commit to chriseth/yultsur that referenced this pull request Aug 5, 2022
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