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

[Core] Prevent copying of SelfList and SelfList::List #85180

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

AThousandShips
Copy link
Member

Copying of these types is unsafe and should be detected

Also removed unnecessary constructors for TileMap DebugQuadrant and RenderingQuadrant which used copying of SelfList::List

Bug identified while researching:

Unable to reliably replicate this crash so can't confirm this is a fix, will continue to investigate that fix

@AThousandShips
Copy link
Member Author

The TileMap data types should not be a problem with this as they are RefCounted and therefore never invoke their copy constructor

Copying of these types is unsafe and should be detected

Also removed unnecessary constructors for `TileMap` `DebugQuadrant` and
`RenderingQuadrant` which used copying of `SelfList::List`
@akien-mga akien-mga merged commit 81f618d into godotengine:master Jan 8, 2024
15 checks passed
@AThousandShips AThousandShips deleted the self_list_fix branch January 8, 2024 11:11
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

@groud
Copy link
Member

groud commented Jan 8, 2024

The TileMap data types should not be a problem with this as they are RefCounted and therefore never invoke their copy constructor

Did you verify that ? Because, as far as I remember, those constructors were needed to avoid several issues. Though I guess forbidding the copy as you did would probably trigger a compilation error 🤔 .

Anyway, that was just to mention this is probably something we need to keep an eye on.

I am not sure why

@AThousandShips
Copy link
Member Author

I did both by compilation and by that there's no such semantics in the engine, they're created and shared, not copied, were these data elements always ref counted? Looks like a remnant of a version not using that

@groud
Copy link
Member

groud commented Jan 8, 2024

were these data elements always ref counted? Looks like a remnant of a version not using that

Ah that's a good point, that might be it! Good call.

@AThousandShips
Copy link
Member Author

If the two quadrant types were built from the old TileMapQuadrant then that's probably why, it was non-ref counted and had a copy assignment with the same comment 🙂

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

Successfully merging this pull request may close these issues.

4 participants