Revert "Unstable book options parser"#154488
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
rustbot
added
S-waiting-on-review
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Sorry to be the bearer of bad news. |
Zalathar
added
the
A-contributor-roadblock
|
The main concern that was also pointed by @clubby789 is that the change might cause maintenance issues. |
There was a problem hiding this comment.
I do think the reverted functionality is potentially useful, and I'm not fundamentally opposed to a revised version of it landing in the future, if the relevant concerns are addressed.
The include! I agree seems quite annoying for navigation at least (not to mention trying to change unstable options), on top of the very complex parsing in unstable-book-gen.
I'm in favor of a revert to not have to be time-pressured to resolve some of these, and I don't really want to have a series of fix-forwards in this situation.
|
For any reland attempt, I'd suggest first split out unstable-book-gen test logistics and hook up its testing to CI. Then, I'd like us to discuss if it's worth the maintenance overhead, and even if so, can we somehow lower it (esp. if there's 2 syn vs decl macro versions that must be kept in sync, which can be quite annoying). I'm going to go ahead and approve this revert as a bootstrap/compiler reviewer. |
rust-bors
bot
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - #154357 (uefi: extend comment for TcpStream Send impl) - #154410 (Clean up the API for opening/checking incremental-compilation files ) - #154081 (format safety doc of Rc/Arc::from_raw/from_raw_in) - #154110 (Change "error finalizing incremental compilation" text and emit it as a note, not a warning) - #154196 (Make `Ipv6Addr::multicast_scope()` exhaustive) - #154221 (`vec::as_mut_slice()`: use lowercase "isize" in safety comment) - #154234 (Use common Timestamp impl in Hermit (attempt 2)) - #154396 (chore(deps): update rust crate tar to v0.4.45) - #154488 (Revert "Unstable book options parser")
Rollup of 9 pull requests Successful merges: - #154357 (uefi: extend comment for TcpStream Send impl) - #154410 (Clean up the API for opening/checking incremental-compilation files ) - #154081 (format safety doc of Rc/Arc::from_raw/from_raw_in) - #154110 (Change "error finalizing incremental compilation" text and emit it as a note, not a warning) - #154196 (Make `Ipv6Addr::multicast_scope()` exhaustive) - #154221 (`vec::as_mut_slice()`: use lowercase "isize" in safety comment) - #154234 (Use common Timestamp impl in Hermit (attempt 2)) - #154396 (chore(deps): update rust crate tar to v0.4.45) - #154488 (Revert "Unstable book options parser")
Rollup merge of #154488 - Zalathar:unstable, r=jieyouxu Revert "Unstable book options parser" - Reverts #154070 This reverts commit 6fd8466, reversing changes made to fda6d37. --- The changes in #154070 are well-intentioned, but in their current form they place an unreasonable maintenance burden on the affected part of the compiler, out of proportion to the benefit gained. - Moving the unstable options to a separate `include!` file is marginally more convenient for the unstable-book-gen tool, but a big hassle for compiler contributors looking to read or modify the list of unstable options. It breaks normal file layout conventions, interferes with navigation, and interferes with IDE features, all without ever being truly necessary. - The extra `syn`-based parser in unstable-book-gen is quite complicated, almost entirely undocumented, and seemingly buggy. That makes it extremely difficult to understand or modify, which in turn makes it effectively impossible to ever change the `options!` macro on the compiler side. - The new tests in unstable-book-gen don't appear to be hooked up to run in CI. - The compiler changes don't appear to have been approved by a T-compiler reviewer. Before contemplating a revert, I tried to put together a fix-forward to resolve or mitigate some of these issues. But unstable-book-gen in its current state makes that impractical, so unfortunately I believe that the only way forward is to revert the original changes. --- I do think the reverted functionality is potentially useful, and I'm not fundamentally opposed to a revised version of it landing in the future, if the relevant concerns are addressed. But I feel strongly that the current version does not justify the real and troublesome maintenance burden that it imposes on compiler contributors.
5 participants
This reverts commit 6fd8466, reversing changes made to fda6d37.
The changes in #154070 are well-intentioned, but in their current form they place an unreasonable maintenance burden on the affected part of the compiler, out of proportion to the benefit gained.
Moving the unstable options to a separate
include!file is marginally more convenient for the unstable-book-gen tool, but a big hassle for compiler contributors looking to read or modify the list of unstable options. It breaks normal file layout conventions, interferes with navigation, and interferes with IDE features, all without ever being truly necessary.The extra
syn-based parser in unstable-book-gen is quite complicated, almost entirely undocumented, and seemingly buggy. That makes it extremely difficult to understand or modify, which in turn makes it effectively impossible to ever change theoptions!macro on the compiler side.The new tests in unstable-book-gen don't appear to be hooked up to run in CI.
The compiler changes don't appear to have been approved by a T-compiler reviewer.
Before contemplating a revert, I tried to put together a fix-forward to resolve or mitigate some of these issues. But unstable-book-gen in its current state makes that impractical, so unfortunately I believe that the only way forward is to revert the original changes.
I do think the reverted functionality is potentially useful, and I'm not fundamentally opposed to a revised version of it landing in the future, if the relevant concerns are addressed.
But I feel strongly that the current version does not justify the real and troublesome maintenance burden that it imposes on compiler contributors.