Skip to content
This repository has been archived by the owner on Dec 21, 2018. It is now read-only.

Re issue #138 - Added some Doxygen comments for the API #174

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

Re issue #138 - Added some Doxygen comments for the API #174

wants to merge 1 commit into from

Conversation

eyalroz
Copy link
Contributor

@eyalroz eyalroz commented Oct 22, 2018

Following the discussion on issue 138, here's a hopefully-template-compliant PR adding documentation to some API functions and data types (not all of them).

There are other some minor tweaks such as removal of some /* ---- */s, spelling and rephrasing.

@eyalroz eyalroz changed the title Regarding issue #138 - Added some Doxygen for the API Re issue #138 - Added some Doxygen for the API Oct 22, 2018
@eyalroz eyalroz changed the title Re issue #138 - Added some Doxygen for the API Re issue #138 - Added some Doxygen comments for the API Oct 22, 2018
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@eyalroz
Copy link
Contributor Author

eyalroz commented Oct 22, 2018

@GPUtester : If somebody started to do that, they need to restart, since I just amended the commit myself after finding some typos.

@jrhemstad
Copy link
Contributor

Is this coordinated with prior work being done here? #156

GDF_DATE32, /**< int32_t days since the UNIX epoch */
GDF_DATE64, /**< int64_t milliseconds since the UNIX epoch */
GDF_TIMESTAMP, /**< Exact timestamp encoded with int64 since UNIX epoch (Default unit millisecond) */
GDF_DATE32, /// int32_t days since the UNIX epoch
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting a comment on the same line, as in for a data member or enum requires <.

https://www.stack.nl/~dimitri/doxygen/manual/docblocks.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll fix it. The Eclipse Doxygen plugin accepts just /// without the < as a marker of a same-line comment.

@eyalroz
Copy link
Contributor Author

eyalroz commented Oct 23, 2018

Is this coordinated with prior work being done here? #156

Not as such, no. As a relative newcomer to BlazingDB, I wanted to clear things up for myself about the API - now that I'm going to use it or add to it. I picked @felipeblazing 's brain about some of the API, and read the sources for other parts of it, and this was the result.

@eyalroz
Copy link
Contributor Author

eyalroz commented Oct 23, 2018

@jrhemstad : I've replaced the ///s with ///<s.

///< @todo Is this always in device memory?

gdf_valid_type *valid;
///< a pseudo-column of `size` bits, packed into bytes (with in-byte order from
Copy link
Contributor

Choose a reason for hiding this comment

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

More detail about the bit/byte ordering of the valid mask would be useful for new developers. See the Apache Arrow description of this ordering here: https://arrow.apache.org/docs/memory_layout.html


gdf_size_type null_count;
///< The number of null elements in the column, which is
///< also the number of 1 bits in the `valid` pseudo-column
Copy link
Contributor

Choose a reason for hiding this comment

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

null_count would be the number of 0 bits in the range [0, size).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrhemstad : Right, fixed.

* Added doxygen comments for many data types and functions in the library's API.
* Made some existing doxygen comments conform better to the doxygen documentation template (see issue #198)
* Minor spacing, spelling and formatting tweaks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4 - Needs Rework Additional work is needed migrate to cudf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants