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

scx_stats: Implement macro #stat_doc to autogen doc from stat desc #703

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

minosfuture
Copy link
Contributor

The doc of scx_layered Opt is out of sync.

Implement attribute macro #stat_doc to generate doc from the desc property, and apply #stat_doc to LayerStats and SysStats in scx_layered.

Also fix typo and replace # with "count of" to avoid messing with markdown syntax in the generated rust doc.

Signed-off-by : Ming Yang [email protected]

@minosfuture
Copy link
Contributor Author

Adding screenshot of generated rust doc:

image image

@hodgesds
Copy link
Contributor

Looks like a slight linter error on the workflow for cargo fmt, it's probably a difference in the cargo version. Otherwise, this looks great!

The doc of scx_layered `Opt` is out of sync.

Implement attribute macro #stat_doc to generate doc from the `desc`
property.

Apply #stat_doc to `LayerStats` and `SysStats in scx_layered.

Signed-off-by : Ming Yang <[email protected]>
new_attrs.push(attr);
}

// If a description string was found, add a #[doc = "..."] attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just rename desc field to doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing #[stat(desc = "...")] into #[stat(doc = "...")] won't make it into the generated documentation because doc attribute is actually #[doc = "..."].
Also, desc in stat macro here is put into the StatsMeta, for example, here is the expanded rust code:
image

with the change in this PR, the original struct is extended with document like the following, where /// ... lines are added by the stat_doc macro.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I wonder whether it should be synced the other way then - ie. let scx_stats macro to take the /// comment and turn that into description field. What do you think?

Copy link
Contributor Author

@minosfuture minosfuture Sep 30, 2024

Choose a reason for hiding this comment

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

I think it should be the current way. Documents can be out of rules, while a desc property feels constrained to be one short sentence. In the current way, it's also composable as one can add more doc in comment. So other than sync, it's more an extension to the doc from the desc property. Screenshots for a demo are attached:
image

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. Thanks for the explanation and the PR!

@@ -55,9 +57,9 @@ pub struct LayerStats {
pub load: f64,
#[stat(desc = "fraction of total load")]
pub load_frac: f64,
#[stat(desc = "# tasks")]
#[stat(desc = "count of tasks")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes beacause doc doesn't allow hash sign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's because the hash sign in the front indicates heading in markdown.

@htejun htejun added this pull request to the merge queue Sep 30, 2024
Merged via the queue into sched-ext:main with commit 04648bc Sep 30, 2024
17 of 19 checks passed
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