Skip to content

make counters opt-out rather than opt-in#47

Merged
hawkw merged 2 commits intomainfrom
eliza/no-counters
Mar 8, 2024
Merged

make counters opt-out rather than opt-in#47
hawkw merged 2 commits intomainfrom
eliza/no-counters

Conversation

@hawkw
Copy link
Member

@hawkw hawkw commented Mar 8, 2024

No description provided.

In general, we always want to generate IPC client counters, and only
disable them when building for targets that don't have the memory to
spare. In order to make it harder to accidentally forget to add counters
to a new task, this branch changes the feature flags from opting _in_ to
generating counters by default, to opting _out_ of generating counters
by default.
@hawkw hawkw requested a review from cbiffle March 8, 2024 22:54
pub enum RequestError<E> {
Runtime(#[cfg_attr(feature = "counters", count(children))] E),
Fail(#[cfg_attr(feature = "counters", count(children))] ClientError),
Runtime(#[ount(children)] E),
Copy link
Collaborator

Choose a reason for hiding this comment

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

ount

Copy link
Collaborator

@cbiffle cbiffle left a comment

Choose a reason for hiding this comment

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

Looks good once it compiles! (Need to set up CI in this repo.)

Since the feature flag now disables, rather than enables, counter code
generation, the `idol-runtime` crate's counters support can't be feature
flagged. This is fine, since it just enables trait impls, which can
easily be dead-code eliminated when they're not used.
@hawkw hawkw force-pushed the eliza/no-counters branch from 8949949 to 19dffd3 Compare March 8, 2024 23:00
@hawkw
Copy link
Member Author

hawkw commented Mar 8, 2024

Looks good once it compiles! (Need to set up CI in this repo.)

gah, thanks for fixing the typo! i started on setting up CI in PR #46, but thus far I haven't actually managed to get idol-runtime's userlib dep to build on CI yet: https://github.com/oxidecomputer/idolatry/actions/runs/8206777976/job/22446637010?pr=46

@hawkw hawkw merged commit 094b967 into main Mar 8, 2024
hawkw added a commit that referenced this pull request Mar 8, 2024
I really thought I'd pushed this to #47, but it looks like it somehow
didn't get pushed --- I had the commit staged all along. My bad. Sorry.
@hawkw hawkw mentioned this pull request Mar 8, 2024
hawkw added a commit that referenced this pull request Mar 8, 2024
I really thought I'd pushed this to #47, but it looks like it somehow
didn't get pushed --- I had the commit staged all along. My bad. Sorry.
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