Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent mutability between WriteChunkUninit::producer and ReadChunk::consumer #79

Closed
RamType0 opened this issue Jan 17, 2022 · 5 comments · Fixed by #81
Closed

Comments

@RamType0
Copy link
Contributor

WriteChunkUninit::producer is typed as &'a Producer<T>.
But ReadChunk::consumer is typed as &'a mut Consumer<T>.

With current code, switching their mutability never makes compiler error.
But inconsistent mutability between them seems to be weird.
So I think we should mark both of them as mut or both of them as not mut .

@mgeier
Copy link
Owner

mgeier commented Jan 18, 2022

Thanks, I'm trying to fix this in #81.

The mut is unnecessary because all mutation happens via Cell via interior mutability.

While looking at this, I also found out that two of the four Cell usages are not needed, see #80.

@RamType0
Copy link
Contributor Author

Thanks, I'm trying to fix this in #81.

The mut is unnecessary because all mutation happens via Cell via interior mutability.

While looking at this, I also found out that two of the four Cell usages are not needed, see #80.

Do you know that #80 and #81 is conflicting?
You should mark both of chunk types as mut with #80.

@mgeier
Copy link
Owner

mgeier commented Jan 28, 2022

Do you know that #80 and #81 is conflicting?

No, I missed that, I was only looking at those two PRs separately and didn't realize that they are conflicting.

I've merged #81 and I've closed #80.

@RamType0
Copy link
Contributor Author

Oh, did you take that one?
I think #80 was better.

@mgeier
Copy link
Owner

mgeier commented Jan 31, 2022

I think #80 was better.

I think this is a matter of taste, the generated machine code should be the same in both cases, right?

My argumentation for choosing #81 was:
I cannot get rid of all Cell instances, so why bother removing half of them?
And in case of doubt, immutable is better than mutable, isn't it?

Also, I haven't yet completely given up on #48, which would also not need &mut.

But if you have a good reason for #80, we can still make the change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants