-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add missing docs for some util modules #1024
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly OK. There are some aesthetic problems that can be fixed.
src/util/options.rs
Outdated
@@ -672,88 +700,85 @@ mod gc_trigger_tests { | |||
// Currently we allow all the options to be set by env var for the sake of convenience. | |||
// At some point, we may disallow this and all the options can only be set by command line. | |||
options! { | |||
// The plan to use. | |||
#[doc = "The plan to use."] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the other PR, we should be able to use ///
here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///
may not work here. We can use #[doc]
as we capture the pattern $(#[$outer:meta])*
in the macro. We should be able to use the same pattern to capture things like #[cfg(feature = "a")]
. This seems to be a common way to add attributes for items that are generated by a macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, I was also using #[doc]
to pass attributes (including docs) to macros in the previous PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it works. I changed it to
/// The plan to use. Blah blah. (to see if `///` works)
plan: PlanSelector [env_var: true, command_line: true] [always_valid] = PlanSelector::GenImmix,
Type cargo doc
and the generated HTML contains the part in ///
.
FYI: https://stackoverflow.com/questions/33999341/generating-documentation-in-macros
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Interesting. It works. Rust did not allow ///
in macro_rules
and that was why we have //
in the options!
macro. Probably Rust started to support this at some point. This is great. I will make these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. #[$outer:meta]*
matches ///
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is a step towards #309.
util
modules.util::metadata::side_metadata::helpers/helpers_32
not public.util::reference_processor
not public.options!
macro.