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

Adding docstring for the Allocs #53559

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mariam606
Copy link

@mariam606 mariam606 commented Mar 2, 2024

This is a part of the issue #52725
This PR adds docstring for the Profile: Allocs.

@mariam606
Copy link
Author

@stevengj Hello,
I'm relatively new to open-source contributions. I'm very excited about the opportunity to contribute to this project and I would greatly appreciate any feedback you could provide on my PR. Thanks in advance!

@ViralBShah ViralBShah added the docs This change adds or pertains to documentation label Mar 2, 2024
@ViralBShah
Copy link
Member

Welcome @mariam606


# Fields
- `data::Ptr{BTElement}`: A pointer to the beginning of the backtrace data array.
- `size::Csize_t`: The number of elements in the backtrace data array.
Copy link
Member

Choose a reason for hiding this comment

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

Not clear that we want to document these internal fields — this is not something a user should touch?

Better to document what can be done with this data structure via documented functions, e.g. presumably it is returned by catch_backtrace() and can be used with rethrow and show_backtrace? (I'm just guessing, should be checked.)

Copy link
Member

Choose a reason for hiding this comment

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

If this is an undocumented internal type, then it is pretty much only for people who are reading the source code, in which case a docstring listing the fields isn't very useful.

It includes information about the type of the allocated object, the backtrace associated with the allocation event,
the size of the allocation, the task in which the allocation occurred, and a timestamp marking the allocation time.
This struct is designed to closely mirror the corresponding C structure used internally by Julia,
making it easier to process allocation data in Julia's environment.
Copy link
Member

Choose a reason for hiding this comment

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

Again, would be good to link functions that return a RawAlloc or where you get it, and what functions it can be used with?

Copy link
Member

Choose a reason for hiding this comment

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

See also #53559 (comment)

@stevengj
Copy link
Member

stevengj commented Mar 3, 2024

#52725 is only about adding docstrings for public / exported symbols, which in this case is only the module Allocs itself.

That is, to address #52725 you need to add a docstring at the very top of the file, for module Allocs, summarizing what the module is for and maybe linking key exported methods/types.

You don't need to add docstrings for undocumented internal types and methods, although good documentation is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants