Don't print "private fields" on empty tuple structs#118192
Don't print "private fields" on empty tuple structs#118192bors merged 1 commit intorust-lang:masterfrom
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jsha (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
compiler-errors
left a comment
There was a problem hiding this comment.
As for the changes, this looks correct.
fmease
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
This PR is almost good to go, I only have some suggestions for the added test: I'd turn the “absence checks” (@!has) into “presence checks” (@has) since they're more robust and future-proof. Furthermore, I wonder if the tests Hidden and Private are useful since we already have tests/rustdoc/private-fields-tuple-struct.rs for example.
| @@ -0,0 +1,19 @@ | |||
| // @has issue_118180_empty_tuple_struct/enum.Enum.html | |||
| pub enum Enum { | |||
| // @!has - '//*[@id="variant.Empty"]//h3' '/* private fields */' | |||
There was a problem hiding this comment.
| // @!has - '//*[@id="variant.Empty"]//h3' '/* private fields */' | |
| // @has - '//*[@id="variant.Empty"]//h3' 'Empty()' |
| } | ||
|
|
||
| // @has issue_118180_empty_tuple_struct/struct.Empty.html | ||
| // @!has - '//pre/code' '/* private fields */' |
There was a problem hiding this comment.
| // @!has - '//pre/code' '/* private fields */' | |
| // @has - '//pre/code' 'Empty()' |
|
Thanks for the feedback! Presence rather than absence: sure. I think both are more future-proof in their own way, but if you think this way is better, fine with me. As for the tests, I figured these would be edge cases that might plausibly be affected by a potential fix. But you are right — any such issues should be caught by the rest of the suite. |
fmease
left a comment
There was a problem hiding this comment.
Thanks! One last thing, could you maybe squash the commits? Since there aren't that many changes?
|
|
Test for presence rather than absence Remove redundant tests Issues in those parts will likely be caught by other parts of the test suite.
b0f2721 to
a21d771
Compare
|
Sure. I think this is how I do that, right? ( |
|
@bors r+ rollup |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a4a5c97): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 676.283s -> 675.669s (-0.09%) |
Closes #118180.
While working on this I also noticed that empty struct variants are also rendered rather awkwardly. I'll make another issue for that, since I don't know what the correct rendering would be.