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

Briefly document Vector<> variations #10382

Merged
merged 4 commits into from
Jan 29, 2025
Merged

Conversation

esainane
Copy link
Contributor

@esainane esainane commented Dec 7, 2024

This continues #10329.

Packed*Array aliases seem universally preferred where available, so a link to the list of types seems appropriate.

LocalVector is used sparingly, so mentioning the intent and rough tradeoff involved seems right for an overview.

Bugsquad edit: closes #6259.

@esainane
Copy link
Contributor Author

esainane commented Dec 7, 2024

Please do check my understanding is correct - this is based off my reading of the source, and commentary from #10329, but there may be nuances which I'm missing.

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Approving only for style, and for not seeing anything obviously wrong from my casual understanding of the Godot codebase.

It's good to go as long as it is correct, IMO.

@AThousandShips AThousandShips added enhancement area:contributing Issues and PRs related to the Contributing/Development section of the documentation labels Dec 21, 2024
@esainane
Copy link
Contributor Author

Cheers for all the reviews, and sorry about the delay! I should be able to take another look at this tonight.

@skyace65
Copy link
Contributor

@esainane no rush, but are you planning to continue working on this?

`Packed*Array` aliases seem universally preferred where available, so
a link to the list of types seems appropriate.

`LocalVector` is used sparingly, so mentioning the intent and rough
tradeoff involved seems right for an overview.
@esainane
Copy link
Contributor Author

Sorry for the delay; we live in rather interesting times. :|

I've fully rebased the PR, and tried to incorporate all of the feedback. Current HEAD is an attempted compromise by mentioning both LocalVector and Vector at the start before getting into details, which I think still flows naturally? It's getting a bit late here, so my apologies if there's something silly which slipped through.

@esainane
Copy link
Contributor Author

I suspect this would read better if moved to the "Containers" section which immediately follows "Allocating memory", but wanted to provide an update for existing feedback first.

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.

LGTM

Copy link
Contributor

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Looks correct information-wise now.

Copy link
Contributor

@skyace65 skyace65 left a comment

Choose a reason for hiding this comment

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

Grammar and spelling wise everything looks good

@skyace65 skyace65 merged commit 08745d4 into godotengine:master Jan 29, 2025
1 check passed
@skyace65
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:contributing Issues and PRs related to the Contributing/Development section of the documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pool Vector Dead Link on 4.0
7 participants