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

Replace Event enum with Builder methods #23

Open
jimblandy opened this issue Nov 11, 2022 · 1 comment
Open

Replace Event enum with Builder methods #23

jimblandy opened this issue Nov 11, 2022 · 1 comment
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jimblandy
Copy link
Owner

Maybe we should ditch the idea of an Event value, and simply have hardware, software, cache, and breakpoint methods directly on Builder. Whatever fields the Event enum variants had would become arguments to those methods.

What's important to preserve here is the property that you can't select an event kind without providing all the necessary information to get a meaningful result - like, say, a breakpoint without an address. If we just let people set perf_event_attr fields directly, that would be a risk. But by making people produce an Event the idea was that you'd make sure they'd filled everything in.

Replacing each Event variant with a method on Builder could accomplish the same goal, and remove the pointless indirection of "first we build the Event; then we wait for them to hand it to us (which is the only thing you can do with an Event anyway); then we move the fields into the perf_event_attr struct." Instead, you'd just have the one Builder method that just does the deed.

@jimblandy jimblandy added enhancement New feature or request good first issue Good for newcomers labels Nov 11, 2022
@Phantomical
Copy link
Contributor

As a user of the API, if you do this, please at least ensure that there is still a single method that can be used with all the event types. Being able to call builder.kind(<literally any event type>) is really quite nice and it would be a shame if that went away. Whether that happens through impl Into<Event> or another trait that's implemented for all events doesn't really matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants