Tidy up drop glue notification #19425
Conversation
rustbot
added
the
S-waiting-on-review
ConradIrwin
force-pushed
the
drop-drop-glue-goo
branch
2 times, most recently
from
1a90ad4 to
a6d3abf
Compare
crates/ide/src/hover/render.rs
Outdated
| let rendered_drop_glue = match (drop_info.drop_glue, drop_info.has_dtor) { | ||
| (DropGlue::HasDropGlue, _) | (_, Some(true)) => "has Drop", | ||
| (DropGlue::DependOnParams, _) => "type param may have Drop", | ||
| _ => "no Drop", | ||
| }; |
There was a problem hiding this comment.
"having Drop` sounds weird to me, we might as well use the term drop glue then or significant destructor
There was a problem hiding this comment.
I think I'd prefer to call it significant destructor
There was a problem hiding this comment.
I want to avoid drop glue (at least until rust-lang/reference#873 is resolved), so like the idea of using destructor.
How about:
has destructorno destructortype param may have destructor
What does "significant" mean in this context?
There was a problem hiding this comment.
Actually, reading through https://doc.rust-lang.org/nomicon/destructors.html, we use the phrase "needs Drop" for a type that has drop glue but no impl Drop.
So we could also do:
impl Dropneeds Dropno Droptype params may need Drop
I don't have a strong preference, but do like using Drop as the word that shows up in the source code.
There was a problem hiding this comment.
Slight vote/preference for impl Drop from me.
There was a problem hiding this comment.
impl Drop won't be correct though, these don't necessarily implement Drop when they have drop glue. But sure, needs Drop would probably work
This combines the memory layout and drop information on one line, and makes the wording more succinct. Closes rust-lang#19410
ConradIrwin
force-pushed
the
drop-drop-glue-goo
branch
from
a6d3abf to
498633e
Compare
|
Ok, I've gone with:
|
The current notifications are quite verbose (as discussed in #19410), which
causes some arguably more useful information (like the documentation) to be
harder to see.
One option would just be to disable the option by default, but as proposed
there, it seems like a reasonable middle ground would be to show it but
significantly less verbosely.
This change reduces the amount of information shown (a direct
Dropimpl isrendered the same way as the drop glue that calls
Dropimpls transitively),but I don't know of a scenario where the difference is meaningful.
Closes #19410