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

Compile-time edge module compilation check, native support for ConstMapObserver #2592

Merged
merged 39 commits into from
Nov 4, 2024

Conversation

rmalmain
Copy link
Collaborator

@rmalmain rmalmain commented Oct 8, 2024

follow-up of the patches i introduced in #2520 on edge module.
it improves how map_observer guarantees are done at compile-time when building an edge module, with a nicer error message in case the right function is not called.

@domenukk
Copy link
Member

the trait `Deserialize<'_>` is not implemented for `[T; N]`, which is required by `OwnedMutSizedSliceInner<'_, T, N>: Deserialize<'_>`

@domenukk
Copy link
Member

Status?

@domenukk
Copy link
Member

You can try if the nonnull!() macro works for pointers as well. It should as long as it can be evaluated in the const context

@rmalmain
Copy link
Collaborator Author

rmalmain commented Nov 4, 2024

good to go @domenukk or is there something else to discuss?


impl StdEdgeCoverageClassicModule {
#[must_use]
pub fn builder() -> StdEdgeCoverageClassicModuleBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

At some point it would be beautiful to start documenting anything... Like, what does classic mean?

@@ -150,37 +148,30 @@ where
}
}

impl<T, const N: usize> VariableLengthMapObserver for ConstMapObserver<'_, T, N>
impl<T, const N: usize> ConstantLengthMapObserver<N> for ConstMapObserver<'_, T, N>
Copy link
Member

Choose a reason for hiding this comment

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

How about renaming them to ConstLenMapObserver and VarLenMapObserver btw? Seems shorter and just as to clear.
Random idea of course, feel free to ignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok for me, both work out imho

@domenukk
Copy link
Member

domenukk commented Nov 4, 2024

Feel free to merge after cargo fmt, this is good to go.

@domenukk domenukk merged commit 49ea0b0 into main Nov 4, 2024
99 checks passed
@domenukk domenukk deleted the improve_edge_module_builder branch November 4, 2024 13:34
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.

3 participants