Skip to content

Conversation

bugadani
Copy link
Contributor

As far as I can tell, entries is only used when dropping TypedArenaChunks and their contents. It is already ignored there, if T is not mem::needs_drop, this PR just skips updating it's value.

You can see TypedArenaChunk ignoring the entry count in L71. The reasoning is similar to what you can find in DroplessArena.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

Hmm, this makes me wonder whether we could make the entries field actually be () in case T doesn't need dropping. Any mutations on the field would then require a trait method that in the !needs_drop case just doesn't do anything.

This way we don't get into situations like this where we accidentally use a field that is never touched.

Maybe this is overkill? But it feels kind of nice to me. What do you think?

@bugadani
Copy link
Contributor Author

bugadani commented Oct 20, 2020

I don't think I want to make TypedArena any more complicated, just to save 4-8 bytes in like 3 (well, the query system makes it actually hard to say, how many) places. I think I've already introduced enough complexity :) But I can try and think of a solution if complexity is not a huge concern.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

The bytes aren't really the issue here, but the uses of the field. Though I guess you're right about complexity.

So... I guess leave a comment on that if condition stating why we can ignore this field here safely (because it's never read unless that condition is true).

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

@bors r+ rollup

Thanks!

@bors
Copy link
Collaborator

bors commented Oct 20, 2020

📌 Commit 2705cae has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2020
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 20, 2020
Don't update `entries` in `TypedArena` if T does not need drop

As far as I can tell, `entries` is only used when dropping `TypedArenaChunk`s and their contents. It is already ignored there, if T is not `mem::needs_drop`, this PR just skips updating it's value.

You can see `TypedArenaChunk` ignoring the entry count in L71. The reasoning is similar to what you can find in `DroplessArena`.

r? @oli-obk
This was referenced Oct 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2020
…laumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#78046 (Add codegen test for issue rust-lang#73827)
 - rust-lang#78061 (Optimize const value interning for ZST types)
 - rust-lang#78070 (we can test std and core panic macros together)
 - rust-lang#78076 (Move orphan module-name/mod.rs files into module-name.rs files)
 - rust-lang#78129 (Wrapping intrinsics doc links update.)
 - rust-lang#78133 (Add some MIR-related regression tests)
 - rust-lang#78144 (Don't update `entries` in `TypedArena` if T does not need drop)
 - rust-lang#78145 (Drop unneeded `mut`)
 - rust-lang#78157 (Remove unused type from librustdoc)

Failed merges:

r? `@ghost`
@bors bors merged commit ad218f9 into rust-lang:master Oct 20, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 20, 2020
@bugadani bugadani deleted the elements-nodrop branch October 21, 2020 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants