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.

Sign up for GitHub

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

Jump to bottom

[stdlib] Rename List.size to List._len and refactor usage of the field to use the public API #3814

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

[stdlib] Rename List.size to List._len and refactor usage of the field to use the public API #3814

wants to merge 29 commits into from

Conversation

martinvuyk
Copy link
Contributor

@martinvuyk martinvuyk commented Nov 27, 2024 •
edited
Loading

Rename List.size to List._len and refactor usage of the field to use the public API.

Details:

@martinvuyk martinvuyk requested a review from a team as a code owner November 27, 2024 00:39
@martinvuyk
Copy link
Contributor Author

I'll leave this as draft until a decision is made of whether to deprecate accessing the fields directly. Since this implementation will have issues when trying to access the field with an immediate operation e.g. some_list.length += 1, which was solved only for some_list.size += 1 by making size the default getattr field name

@martinvuyk martinvuyk marked this pull request as draft December 2, 2024 17:48
@JoeLoser
Copy link
Collaborator

JoeLoser commented Dec 4, 2024

Personal opinion: I'd like it if we could just deprecate letting people modify the length variable directly (removing setters and getters), and make them use the public API.

Let's do that and remove the setters/getters.

Can you please also add a changelog entry so people understand their compiler errors as a result of this change if they are accessing the length field directly of List?

@JoeLoser JoeLoser added the waiting for response Needs action/response from contributor before a PR can proceed label Dec 4, 2024
@martinvuyk martinvuyk force-pushed the make-list-size-private branch from 6e070d8 to 2c7857d Compare December 4, 2024 13:52
@martinvuyk
fix pixi.toml diff
1cc618e 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix pixi.toml diff
4c4e1fe 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix details
6c178fb 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix details
8e88d34 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk martinvuyk changed the title [stdlib] Rename List.size to List._len adding getattr and setattr for size [stdlib] Rename List.size to List._len and refactor usage of the field to use the public API Dec 4, 2024
@martinvuyk
fix details
5bc5722 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk martinvuyk marked this pull request as ready for review December 4, 2024 14:53
@martinvuyk martinvuyk requested a review from JoeLoser December 4, 2024 14:58
@martinvuyk martinvuyk marked this pull request as draft December 10, 2024 22:44
@martinvuyk martinvuyk mentioned this pull request Dec 10, 2024
Closed
modularbot added a commit that referenced this pull request Dec 21, 2024
@modularbot @martinvuyk @ConnorGray
[External] [stdlib] Add List[Scalar[D]].extend() from SIMD and `S…
6eb0b89 
…pan[Scalar[D]]` (#52584)

[External] [stdlib] Add `List[Scalar[D]].extend()` from `SIMD` and
`Span[Scalar[D]]`

Add `List[Scalar[D]].extend()` from `SIMD` and `Span[Scalar[D]]`

Split off from #3814. This is needed to enable efficient
appending of scalar value sequences to a `List` without having to resort
to `UnsafePointer` manually.

ORIGINAL_AUTHOR=martinvuyk
<[email protected]>
PUBLIC_PR_LINK=#3854

Co-authored-by: martinvuyk <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Closes #3854
MODULAR_ORIG_COMMIT_REV_ID: 7d0c724497ba0671ae660f4de5758d6c4baad7bc
msaelices pushed a commit to msaelices/mojo that referenced this pull request Dec 22, 2024
@modularbot @martinvuyk
@ConnorGray @msaelices
[External] [stdlib] Add List[Scalar[D]].extend() from SIMD and `S…
c84c329 
…pan[Scalar[D]]` (#52584)

[External] [stdlib] Add `List[Scalar[D]].extend()` from `SIMD` and
`Span[Scalar[D]]`

Add `List[Scalar[D]].extend()` from `SIMD` and `Span[Scalar[D]]`

Split off from modular#3814. This is needed to enable efficient
appending of scalar value sequences to a `List` without having to resort
to `UnsafePointer` manually.

ORIGINAL_AUTHOR=martinvuyk
<[email protected]>
PUBLIC_PR_LINK=modular#3854

Co-authored-by: martinvuyk <[email protected]>
Co-authored-by: Connor Gray <[email protected]>
Closes modular#3854
MODULAR_ORIG_COMMIT_REV_ID: 7d0c724497ba0671ae660f4de5758d6c4baad7bc
@martinvuyk martinvuyk marked this pull request as ready for review January 2, 2025 13:27
@arthurevans
Copy link
Collaborator

Not sure what the status of this PR is, but left a suggestion for the changelog entry just in case.

@skongum02 skongum02 deleted the branch modular:main January 29, 2025 18:58
@skongum02 skongum02 closed this Jan 29, 2025
@skongum02 skongum02 reopened this Jan 29, 2025
@skongum02 skongum02 changed the base branch from nightly to main January 29, 2025 20:43
@martinvuyk
Copy link
Contributor Author

Hi @arthurevans thanks for the input, you're totally right that this needed some examples :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arthurevans arthurevans arthurevans left review comments

@ConnorGray ConnorGray ConnorGray left review comments

@JoeLoser JoeLoser Awaiting requested review from JoeLoser

Assignees
No one assigned
Labels
waiting for response Needs action/response from contributor before a PR can proceed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martinvuyk @JoeLoser @arthurevans @ConnorGray @skongum02