Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Create a more rigid overseer builder pattern that fails at compile time #4753

Merged
merged 44 commits into from
Feb 9, 2022

Conversation

vstakhov
Copy link
Contributor

This pull requests addresses #3772

The general idea is to generalise all fields arguments and fix them in two states:

  • Uninit<T> for uninitialised fields
  • Init<T> for initialised fields

With this approach, each setter function accepts builder template with Uninit<Field> and returns a builder where this field is changed to Init<Field>. Subsequently build function accepts all templated fields to be Init<T> and fails to compile if any of the required fields is missing.

@vstakhov vstakhov added A3-in_progress Pull request is in progress. No review needed at this stage. A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. labels Jan 20, 2022
@vstakhov vstakhov requested a review from drahnr January 20, 2022 13:26
@vstakhov vstakhov self-assigned this Jan 20, 2022
@github-actions github-actions bot removed the A0-please_review Pull request needs code review. label Jan 20, 2022
@vstakhov vstakhov added the B0-silent Changes should not be mentioned in any release notes label Jan 20, 2022
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Good start! It requires some additional attention on the way things are represented, and - as you already said - a round of cleanups.

node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
node/overseer/overseer-gen/proc-macro/src/impl_builder.rs Outdated Show resolved Hide resolved
@vstakhov vstakhov added the A0-please_review Pull request needs code review. label Jan 24, 2022
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Superficial pass

)*
};
quote! {
impl <InitStateSpawner, #additional_baggage_generic #( #subsystem_passthrough_state_generics, )* #( #impl_baggage_state_generics, )* >
Copy link
Contributor

Choose a reason for hiding this comment

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

...and add it here instead

Comment on lines +516 to 518
let subsystem_string = String::from(stringify!(#subsystem_name));
// Convert owned `snake case` string to a `kebab case` static str.
let subsystem_static_str = Box::leak(subsystem_string.replace("_", "-").into_boxed_str());
Copy link
Contributor

@drahnr drahnr Jan 27, 2022

Choose a reason for hiding this comment

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

note to myself: this could be extracted as generated &'static str rather than the adhoc string generation and leaking it.

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

a few micro nits that need to be addressed, other than that 👍

@drahnr drahnr merged commit e4ffa3e into master Feb 9, 2022
@drahnr drahnr deleted the vstakhov-rigid-overseer-builder branch February 9, 2022 16:01
@coderobe coderobe mentioned this pull request Mar 17, 2022
13 tasks
@ordian ordian added the T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance. label Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants