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

[BUG] The field storing the number of items in a List or Dict should be named "length", not "size" #3511

Open
nmsmith opened this issue Sep 20, 2024 · 7 comments
Labels
enhancement New feature or request mojo-repo Tag all issues with this label

Comments

@nmsmith
Copy link
Contributor

nmsmith commented Sep 20, 2024

Bug description

The field List.size should probably be renamed to List.length, because:

  • That's what Python's len() function returns (it returns the # of elements in any collection)
  • "length" is used in everyday English when talking about lists

A similar reasoning applies to Dict.size. The expression len(my_dict) obtains the number of elements, so we should probably be referring to the number of elements as the "length" of the dictionary.

Also, the term "size" is more often used to refer to memory consumption, e.g. C++'s sizeof operator returns the number of bytes that an object consumes. It would be good to avoid this conflation.

Steps to reproduce

N/A

System information

No response

@nmsmith nmsmith added bug Something isn't working mojo-repo Tag all issues with this label labels Sep 20, 2024
@lattner
Copy link
Collaborator

lattner commented Sep 20, 2024

+1, this makes sense to me. @JoeLoser wdyt?

@JoeLoser
Copy link
Collaborator

+1, this makes sense to me. @JoeLoser wdyt?

+1 to renaming the field names here for consistency (using length instead of size). We should also signal that they are meant to be private as well by prefixing these with an _. It would be bad for users to do something like

var xs = List[String]()
xs.size = 10 # oops

# When we run the destructor for `xs` here, we'd try to free uninitialized memory

Our team can do all this internally and fix up all the uses.

@JoeLoser JoeLoser removed the bug Something isn't working label Sep 20, 2024
@JoeLoser JoeLoser added the enhancement New feature or request label Sep 20, 2024 — with Linear
@nmsmith
Copy link
Contributor Author

nmsmith commented Sep 20, 2024

Also, if we're renaming the word "size", it might also make sense to rename the Sized trait (that includes the __len__ method) to something different. Notably, in Rust, the Sized trait means something completely different. Unfortunately there isn't really an English adjective that means "has a length", but the name Countable seems very accurate.

(I'll open a different issue for this if desired.)

Copy link
Collaborator

JoeLoser commented Sep 20, 2024

We definitely had similar thoughts when we introduced the Sized trait since the "able" suffix naming doesn't quite work with the (currently) named Sized, e.g. Lengthable isn't great. Mind opening a separate GH issue for that and we can discuss with the community, please?

@nmsmith
Copy link
Contributor Author

nmsmith commented Sep 21, 2024

That said, given Mojo already invented the adjectives Intable and Stringable, maybe Lengthable would be fine. 🤷‍♀️

@soraros
Copy link
Contributor

soraros commented Sep 21, 2024

Let's use _len: it's short, less specific (can be applied to types where a literal "length" doesn't make sense, but still maintains parity with len), and somewhat "ugly", indicating its internal nature.

the name Countable seems very accurate.

+1. Or HasLen.

@nmsmith
Copy link
Contributor Author

nmsmith commented Sep 21, 2024

For discussing the Sized trait, I opened #3514.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

4 participants