Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix doc cfg on reexports #101006
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
Fix doc cfg on reexports #101006
Changes from all commits
01d64f5
2ed9454
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It looks like this is going to wind up copying real cfg, and not just doc(cfg). Do we want that? The way
libc
uses re-exports, this seems like it would make their lives harder, not easier.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.
We need to keep both until we call
merge_attrs
because it internally callsAttributesExt::cfg
which makes the "filtering" depending on thedoc_cfg
and thedoc_auto_cfg
features (which is why it returns a tuple of two and not just aast::Attributes
).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.
But here's a screenshot of what the
libc
looks like, when it's built from nightly and#![feature(doc_auto_cfg)]
is added:And here's a screenshot of what
libc
looks like, when it's built from your branch and#![feature(doc_auto_cfg)]
is added: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.
Sorry I'm a bit lost: isn't it how it should be? The reexports should display the
cfg
features of the reexported item, no?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.
The docs are claiming things that aren't true:
This thing claims that
c_short
is present on Unix and absent on Windows. Butc_short
is definitely present on Windows; it's just implemented separately.Basically, if this PR is going to be merged, then
doc_auto_cfg
should probably be off by default. Otherwise, a lot of crates with platform-specific implementations of common interfaces (libc
is just a very high-profile example) are going to have wrong documentation.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.
I don't agree on that but it confirms that allowing to disable/enable doc_auto_cfg is a must have.
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.
As long as we agree that doc_auto_cfg needs to be optional, I’m fine with merging this.
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.
I think this PR just highlighted it. The discussion now will be either it's enabled by default or not and what the attribute to enable/disable should look like.