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

[change] Rename the data buffer from data to _buffer #136

Merged

Conversation

forFudan
Copy link
Collaborator

The data buffer of NDArray is an unsafepointer. We should not expose it to the users. So it is renamed to _buffer with an underscore.

@MadAlex1997
Copy link
Collaborator

The data buffer of NDArray is an unsafepointer. We should not expose it to the users. So it is renamed to _buffer with an underscore.

All this would really do is hide the UnsafePointer declaration in the documentation of NDArray, mojo does not have a public/private interface system to stop users from touching things.

I am not opposed to a name change since data is kind of vague, perhaps pointer or ptr. I feel that it is valid for an advanced user to want to take ownership of the ptr from NDArray, although I struggle to think of a good reason to do that.

@forFudan
Copy link
Collaborator Author

All this would really do is hide the UnsafePointer declaration in the documentation of NDArray, mojo does not have a public/private interface system to stop users from touching things.

Yeah, but we have the Pythonic convention that things with single underscore means "private" and ordinary users should not touch it unless knowing the consequence :D

With this name, users will understand that they need to be cautious and do not accidentallyy change the data buffer.

Actually it is also used for the buffer of the String type in the standard library.

@forFudan
Copy link
Collaborator Author

For String type:

struct String(
...
):
    """Represents a mutable string."""

    # Fields
    alias _buffer_type = List[UInt8, hint_trivial_type=True]
    var _buffer: Self._buffer_type

@shivasankarka
Copy link
Collaborator

  • I am on board for this change, but I think _ptr might be a simple option. Although it doesn't change a lot, following the python convention for now might be a good idea.
  • We already have some methods that exposes the UnsafePointer which I think should be allowed for advanced user, so something simple as _ptr seems good.

@forFudan
Copy link
Collaborator Author

forFudan commented Oct 22, 2024

We already have some methods that expose the UnsafePointer which I think should be allowed for advanced users, so something simple as _ptr seems good.

For sake of simplicity, it can be named as _buf, but _ptr is not a proper name.

On the one hand, ptr stands for the type of the underlying data, but not the purpose (nature) of the data. There can be multiple unsafe pointers in a structure. Using ptr is ambiguous.

On the other hand, for C users, a structure pointer points to the memory address where the structure is stored, not the buffer of the structure. One still needs to do ptr->buffer to get to the underlying buffer. Using ptr for the buffer would be misleading.

That is why I think _buf would be better.

@shivasankarka
Copy link
Collaborator

shivasankarka commented Oct 22, 2024

@forFudan Yep, _buf seems fine to me. Please update it so that I canl merge it.

@shivasankarka shivasankarka merged commit 8b163e5 into Mojo-Numerics-and-Algorithms-group:pre-0.4 Oct 22, 2024
3 checks passed
@forFudan forFudan deleted the init branch October 22, 2024 11:04
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.

3 participants