-
Notifications
You must be signed in to change notification settings - Fork 8
Depub #309
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
Depub #309
Conversation
|
If you're OK with me doing so, I suggest I rebase this against master and force push when #308 has merged before you review? |
| } | ||
| } | ||
|
|
||
| pub(crate) fn start_tracing() -> ThreadTracer { |
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'm not sure if this is right. Why would starting tracing be a testing feature?
It's probably because your tool build with hardware tracing rustflags (and not software tracing)?
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.
Similarly for the other bits in this file.
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's probably because your tool build with hardware tracing rustflags (and not software tracing)?
Yes, I used hw. I didn't realise that there'd be a difference here with sw? Would a bors try flush this out?
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 didn't realise that there'd be a difference here with sw?
My guess is that compile-time guards like this would give inaccurate results if the oracle uses only -C tracer=hw.
Ideally your oracle would include both kinds of tracing, but read on...
Would a bors try flush this out?
No, because we don't yet test software tracing with cranelift. I'm not sure it's ready yet, but @bjorn3 is the person to ask.
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.
Having looked at this again, I agree that applying this to swt.rs is incorrect. Fixed in 0703d94.
|
I think this is ready for squashing (and may as well sync the branch while we are here). |
|
OK, as soon as #308 merges, I'll sync and force push. |
These were found by a bodged version of depub.
This has to be handled a bit delicately, as repeatedly compiling ykrustc if there are errors sometimes fails/succeeds in ways that I don't really understand. It's possible that more depub'ing could be done, but something is better than nothing.
|
Rebased and squashed. |
|
bors r+ |
|
Build succeeded: |
Needs #308 to be merged first. I'm raising this as useful context for softdevteam/depub#1.
This PR uses https://github.com/softdevteam/depub/ to reduce the visibility of quite a lot more items in yk. In some cases, we only slightly reduce visibility (
pub(crate)topub(super)) but there are a reasonable number where we get rid ofpubentirely. We're also able to gate a small number of items behind#[cfg(feature="testing")]to make clear when something is only needed for our (unusual) test setup. Overall, this PR has no functional change, but should make it a tiny little bit easier to understand the code base.