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

Add const lvalue ref to core/* container parameters #86966

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

Muller-Castro
Copy link
Contributor

@Muller-Castro Muller-Castro commented Jan 8, 2024

Adds const lvalue reference to the core container types specified in the documentation that are declared in function parameters in core/* files

initial: #51156
editor/*: #88368
modules/*: #88915
platform/*: #88971
servers/*: #88972
scene/*: #88974
others: #88975

@Muller-Castro
Copy link
Contributor Author

Ok I'll leave the virtual methods that are issuing build errors out of this PR and put then in another PR

@AThousandShips
Copy link
Member

Or at minimum remove all unrelated changes from other parts, because they make up a lot of it 🙂

@Muller-Castro
Copy link
Contributor Author

Muller-Castro commented Jan 8, 2024

You mean remove all folders except core and remove core's virtual methods that aren't compiling?

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial glance, will go over properly when the scope has been reduced 🙂

core/core_bind.cpp Outdated Show resolved Hide resolved
core/core_bind.cpp Outdated Show resolved Hide resolved
core/io/dir_access.cpp Outdated Show resolved Hide resolved
@Muller-Castro Muller-Castro force-pushed the value2ref-core branch 3 times, most recently from 90c1eb0 to 4c54ecd Compare January 9, 2024 03:21
@Muller-Castro
Copy link
Contributor Author

Those non-core/* are inevitable for all core/* files to build

@AThousandShips
Copy link
Member

Will take a look, thank you and sorry for the inconvenience (and you'll get a bonus of more PRs!)

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the note below this looks good and safe to me

Copy link
Member

@AThousandShips AThousandShips Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need a second eye on this, as it's very core and specific, unsure about possible side effects, they're my only question mark as it were, should be fine but it's so central that possible side effects could be major

It's simply too big a question mark for me alone (not that I should be the only one reviewing this, but marking it as a point of extra attention)

@akien-mga akien-mga changed the title Add const lvalue ref to core/* container parameters Add const lvalue ref to core/* container parameters Jan 11, 2024
@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Jan 17, 2024
@AThousandShips
Copy link
Member

You have a merge commit in your history, please use rebasing instead, do this by:

git reset --hard HEAD^
git rebase -i master
git push --force

@akien-mga
Copy link
Member

Sorry for the delay. Looks good to me overall, needs a rebase and then this should be ready to merge if nobody has any outstanding concerns about the change.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 14, 2024

I'd like an eye by a core maintainer familiar with the method_ptrcall.h file, just to ensure there's no weirdness with that, otherwise I think this looks good

CC @reduz @dsnopek perhaps

@dsnopek
Copy link
Contributor

dsnopek commented Feb 14, 2024

The changes in method_ptrcall.h seem fine to me! The const is added to Vector's that we're only reading from.

@akien-mga akien-mga merged commit ef5d6cc into godotengine:master Feb 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

6 participants