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

Add spans and more include guidelines to libcudf developer guide [skipci] #8931

Merged
merged 2 commits into from
Aug 11, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion cpp/docs/DEVELOPER_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,16 @@ The following guidelines apply to organizing `#include` lines.
* Always check that includes are only necessary for the file in which they are included.
Try to avoid excessive including especially in header files. Double check this when you remove
code.
* Use quotes `"` to include local headers from the same relative source directory. This should only
occur in source files and non-public header files. Otherwise use angle brackets `<>` around
included header filenames.
* Avoid relative paths with `..` when possible. Paths with `..` are necessary when including
(internal) headers from source paths not in the same directory as the including file,
because source paths are not passed with `-I`.
* Avoid including library internal headers from non-internal files. For example, try not to include
headers from libcudf `src` directories in tests or in libcudf public headers. If you find
yourself doing this, start a discussion about moving (parts of) the included internal header
to a public header.

# libcudf Data Structures

Expand Down Expand Up @@ -246,7 +256,31 @@ An *immutable*, non-owning view of a table.

### `cudf::mutable_table_view`

A *mutable*, non-owning view of a table.
A *mutable*, non-owning view of a table.

## Spans

libcudf provides `span` classes that mimic C++20 `std::span`, which is a lightweight
view of a contiguous sequence of objects. libcudf provides two classes, `host_span` and
`device_span`, which can be constructed from multiple container types, or from a pointer
(host or device, respectively) and size, or from iterators. `span` types are useful for defining
generic (internal) interfaces which work with multiple input container types. `device_span` can be
constructed from `thrust::device_vector`, `rmm::device_vector`, or `rmm::device_uvector`.
`host_span` can be constructed from `thrust::host_vector`, `std::vector`, or `std::basic_string`.

If you are definining internal (detail) functions that operate on vectors, use spans for the input
vector parameters rather than a specific vector type, to make your functions more widely applicable.
Comment on lines +271 to +272
Copy link
Contributor

@vuule vuule Aug 5, 2021

Choose a reason for hiding this comment

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

Should we include the difference between span<T> const and span<T const> here?
Something like "For input span function parameters, use span<T const>, instead of span<T> const".

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will add that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.


When a `span` refers to immutable elements, use `span<T const>`, not `span<T> const`. Since a span
is lightweight view, it does not propagate `const`-ness. Therefore, `const` should be applied to
the template type parameter, not to the `span` itself. Also, `span` should be passed by value
because it is a lightweight view. APIS in libcudf that take spans as input will look like the
following function that copies device data to a host `std::vector`.

```c++
template <typename T>
std::vector<T> make_std_vector_async(device_span<T const> v, rmm::cuda_stream_view stream)
```

## `cudf::scalar`

Expand Down