-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Merged by Bors] - Add From<EntityMut> for EntityRef (fixes #5459) #5461
Closed
Closed
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much rather that this use the safe path.
In that case, I think we should have it as a method instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify? Are you asking to use EntityRef::new, or something else entirely?
It seems as though the alternative "safe path" is just doing another lookup for no reason. Why is it "safe" and the proposed change is not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely should use
EntityRef::new
(which also should probably be markedunsafe
, since theLocation
being 'valid' is "relied" upon for safety).Looking more closely (not on mobile), I do think we can get away with avoiding the other lookup.
I'm still not sure it should be a
From
impl either really, given that it should still be valid so long as it takes an exclusive refernece to self.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide justifications for your proposals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah; I'm not sure I agree with you here. The invariants of
EntityMut
andEntityRef
will always be tied closely together; converting into the immutable form should always be straightforward and safe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DJMcNab can this be marked as resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't like that it uses the struct syntax, but I can see that others disagree. I do agree that this entire module has insufficient safety comments, but I hardly see how that's justification to make it worse.
But like I say, that seems to be fighting against the tide.
Fwiw, I'm still not convinced that the owned to owned path is the most useful/idiomatic, or that it should be a From impl. I know it's not quite the same, but
std::sync::Weak
doesn't have aFrom<std::sync::Arc>
impl.But again, I've been overruled.
I guess we can resolve it, although I can't say I'm super pleased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on using
EntityRef::new
rather than building the struct manually. It would help to not miss when adding controls/safety to thenew
later. Building the struct here is only possible because the two are in the same module, but they should be considered to be in different module for privacy/access to fieldsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm convinced; let's go with
EntityRef::new
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0163bdd