Skip to content

Conversation

@bgreni
Copy link
Contributor

@bgreni bgreni commented Mar 19, 2025

@lsh Since you mentioned you would make an internal ticket for this.

Add a fill constructor and method for List.

@bgreni bgreni requested a review from a team as a code owner March 19, 2025 22:22
Signed-off-by: Brian Grenier <grenierb96@gmail.com>
@JoeLoser
Copy link
Collaborator

JoeLoser commented Mar 20, 2025

@lsh already has an internal PR up from yesterday for this — sorry for any confusion there. He opted not to have a fill method and instead just

    fn __init__(out self, *, length: Int, fill: T):
        """Constructs a list with the given capacity.
        Args:
            length: The requested length of the list.
            fill: The element to fill each element of the list.
        """
        self = Self()
        self.resize(length, fill)

I'll let him work with you on deciding whether we want a dedicated fill method or whether the constructor only suffices. I think just the constructor is sufficient, WDYT?

@gryznar
Copy link
Contributor

gryznar commented Mar 20, 2025

I am curious why not to use UInt for length?

@bgreni
Copy link
Contributor Author

bgreni commented Mar 21, 2025

@lsh already has an internal PR up from yesterday for this — sorry for any confusion there. He opted not to have a fill method and instead just

    fn __init__(out self, *, length: Int, fill: T):
        """Constructs a list with the given capacity.
        Args:
            length: The requested length of the list.
            fill: The element to fill each element of the list.
        """
        self = Self()
        self.resize(length, fill)

No worries at all, I wasn't sure if he would have the time! I don't have any issue with excluding a fill method.

I'm not a huge fan of just calling out to resize personally as it involves a couple unnecessary branches, but I might be nitpicking here.

@lsh
Copy link
Contributor

lsh commented Apr 1, 2025

@bgreni I'm not particularly concerned in this case. In my opinion, the effort is probably better spent optimizing resize to be as fast as possible. Either way, my implementation will be out in the next release and we can increment on it as we go.

@gryznar fair point, I changed it to UInt before merging.

@lsh lsh closed this Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants