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

builder...build syntax (experimental) #32

Merged
merged 3 commits into from
May 24, 2023
Merged

Conversation

danlehmann
Copy link
Owner

@danlehmann danlehmann commented May 22, 2023

This patch-set:

  • Breaks up parsing into two phases
  • Emits a new builder, which guarantees that all writable fields are set

The new syntax is optional, so it's hidden behind a feature flag

So far, the parser has been pretty much single-pass: Read the input, then write the output.

That is going to be limiting going forward, so this patch splits the parsing out into parse_field, which
returns FieldDefinition. FieldDefinition has all the same fields as what we needed so far, but conveniently bundled
together.
@danlehmann danlehmann changed the title Constructor syntax builder...build syntax (experimental) May 22, 2023
@danlehmann
Copy link
Owner Author

To anyone reviewing: The first 2 patches are just a refactoring to prepare the third one. The third one is the actually new thing.

At the moment, it is required to set all fields in the same order as they are specified. As Rust's const generics become more powerful, this restriction might be lifted.

For the `builder()` to be available, the following has to be true:
- The bitfield has to be completely filled with writable fields (no gaps) OR there has to be a default value specified,
Copy link

Choose a reason for hiding this comment

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

For a register with a mix of read-only and read-write fields, perhaps it would make sense to allow constructing a builder from an existing value, instead of the compile-time default.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good idea! We could allow this as a function while the mask is still 0. Something like this:

let a = A::builder()
  .with_overwritten_raw_value(0x12345678)
  .with_foo(false)
  .with_bar(false)
  .build();

Though I can't quite think of a good name for this function

@johnbwang
Copy link
Collaborator

I personally don't see too much value of having something that requires me to fill out the every field. Most registers I work with don't require me to update everything. In fact, often times registers have sensible defaults and I only need to change a few things.

Copy link
Collaborator

@hecatia-elegua hecatia-elegua left a comment

Choose a reason for hiding this comment

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

Pretty nice idea, iirc from Java, builders were often done like this.
You could remove the prefix with_ here, though.
How does the asm output look?

@danlehmann
Copy link
Owner Author

Pretty nice idea, iirc from Java, builders were often done like this.

Java wouldn't give you any of those compile time guarantees though :)

You could remove the prefix with_ here, though.
I tried that, but I didn't like how it looks completely different compared to the other functions. The upside of keeping the with_xx prefixes is that it's easier to upgrade existing DEFAULT.with_foo(false) to builder().with_foo(false).with_bar(false).build().

I think, consistency here is a win.

How does the asm output look?
Well, it's const, so you can always put the result into a constant and use that. In which case the asm output is a constant :) I'll check the assembly for the case where a variable is being passed it.

@danlehmann
Copy link
Owner Author

How does the asm output look?

Just to make sure, I look at the generated assembly. It seems optimal: If everything passed in is a constant, the result will simply be there. Otherwise, it performs few branches - I didn't see any function calls

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.

5 participants