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

Core Lib Documentation: box module #6651

Merged
merged 15 commits into from
Nov 26, 2024
Merged

Core Lib Documentation: box module #6651

merged 15 commits into from
Nov 26, 2024

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented Nov 13, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @TAdev0)


corelib/src/box.cairo line 5 at r2 (raw file):

//! `Box<T>` provides a way to store a value of type `T` in Cairo VM's _boxed_ segment,
//! leaving only a pointer in the execution segment. This allows for:
//!

This is information about the platform - not the language - shouldn't be the main point of the doc IMO.
(can exist, as a note, not as the main thing)

Code quote:

//! A smart pointer type that stores its value in a dedicated memory segment.
//!
//! `Box<T>` provides a way to store a value of type `T` in Cairo VM's _boxed_ segment,
//! leaving only a pointer in the execution segment. This allows for:
//!

Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/box.cairo line 5 at r2 (raw file):

Previously, orizi wrote…

This is information about the platform - not the language - shouldn't be the main point of the doc IMO.
(can exist, as a note, not as the main thing)

How would you want to see this exactly? I think it's important and thus should not be left out of the documentation (like notes on the memory layouts for Dicts, Arrays, etc).

It's true that now with Cairo Native this is not necessarily true 🤔. Any format you would prefer for these notes?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @enitrat)


corelib/src/box.cairo line 5 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

How would you want to see this exactly? I think it's important and thus should not be left out of the documentation (like notes on the memory layouts for Dicts, Arrays, etc).

It's true that now with Cairo Native this is not necessarily true 🤔. Any format you would prefer for these notes?

As i said - should be as a note in the documentation - but not the top of the definition.

so should probably be at the end, and include the word "note".

I'm really not convinced this is important note for developers, as they should know how to use it (interface) and why to use it (makes moving a value around much cheaper) - knowing how this is actually cheaper under the hood shouldn't be as interesting.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @enitrat)


corelib/src/box.cairo line 5 at r2 (raw file):

Previously, orizi wrote…

As i said - should be as a note in the documentation - but not the top of the definition.

so should probably be at the end, and include the word "note".

I'm really not convinced this is important note for developers, as they should know how to use it (interface) and why to use it (makes moving a value around much cheaper) - knowing how this is actually cheaper under the hood shouldn't be as interesting.

same thing should be true for array and for dict.

Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/box.cairo line 5 at r2 (raw file):

Previously, orizi wrote…

same thing should be true for array and for dict.

I see, thanks

@TAdev0
Copy link
Contributor Author

TAdev0 commented Nov 14, 2024

@orizi @enitrat I added as a // comment the cairo VM memory details. Dont know if its better to keep it or remove it given that Cairo-VM is not the only platform to run Cairo code and I suppose the Cairo library should be platform agnostic

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TAdev0)


corelib/src/box.cairo line 130 at r3 (raw file):

    }
}
// NOTE: A `Box<T>` is a smart pointer type that provides a way to store a value of type `T` in

this can be at the bottom of the top doc IMO.

@TAdev0
Copy link
Contributor Author

TAdev0 commented Nov 15, 2024

@orizi updated

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TAdev0)

@orizi orizi enabled auto-merge November 26, 2024 12:27
@orizi orizi added this pull request to the merge queue Nov 26, 2024
Merged via the queue into starkware-libs:main with commit 03b888d Nov 26, 2024
47 checks passed
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