rustbot
commented
1 year ago This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
cc @rust-lang/compiler @rust-lang/compiler-contributors
oli-obk
apiraino
Proposal
Short summary:
-Cinstrument-coveragean alias for=yesinstead of=all.=allto allow for potential future changes (but leave the current behaviour intact for now).(copied from https://github.com/rust-lang/rust/pull/117199, which implements the proposed change)
The option-value parser for
-Cinstrument-coverage=currently accepts the following stable values:all(implicit value of plain-Cinstrument-coverage)yes,y,on,true(undocumented aliases forall)off(default; same as not specifying-Cinstrument-coverage)no,n,false,0(undocumented aliases foroff)I'd like to rearrange and re-document the stable values as follows:
no(default; same as not specifying-Cinstrument-coverage)n,off,false(documented aliases forno)0(undocumented alias forno)yes(implicit value of plain-Cinstrument-coverage)y,on,true(documented aliases foryes)all(documented as currently an alias foryesthat may change; discouraged but not deprecated)The main changes being:
offtonoalltoyesn,off,false,y,on,true) are explicitly documentedallremains currently an alias foryes, but is explicitly documented as being able to change in the future0remains an undocumented but stable alias fornoWhy?
The choice of
allas the implicit value only really makes sense in the context of the unstableexcept-unused-functionsandexcept-unused-genericsvalues. That arrangement was fine for an unstable flag, but it's confusing for a stable flag whose only other stable value isoff, and will only become more confusing if we eventually want to stabilize other fine-grained coverage option values.(Currently I'm not aware of any plans to stabilize other coverage option values, but that's why I think now is a fine time to make this change, well before anyone actually has to care about it.)
For example, if we ever add support for opt-in instrumentation of things that are not instrumented by
-Cinstrument-coverageby default, it will be very strange for theallvalue to not actually instrument all things that we know how to instrument.Compatibility impact
Because this is not a functional change, there is no immediate compatibility impact. However, changing the documented semantics of
allopens up the possibility of future changes that could be considered retroactively breaking.I don't think this is going to be a big deal in practice, for a few reasons:
allis not a stability-breaking change, as long as it still exists and does something reasonable.-Cinstrument-coverageis mainly used by tools or scripts that can be easily updated if necessary. It's unusual for users to pass the flag directly, because processing the profiler output is complicated enough that tools/scripts tend to be necessary anyway.-Cinstrument-coveragerather than explicitly passing-Cinstrument-coverage=all, so the number of users actually affected by this change is likely to be low, and plausibly zero.Mentors or Reviewers
The work is already done; I'm mostly going through MCP because this is technically a user-visible change (and arguably a breaking one).
Process
The main points of the Major Change Process are as follows:
@rustbot second.-C flag, then full team check-off is required.@rfcbot fcp mergeon either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.