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

Update developer_guide.md with new guidance on quoted internal includes #15238

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
14 changes: 8 additions & 6 deletions cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ and we try to follow his rules: "No raw loops. No raw pointers. No raw synchroni
does use raw synchronization primitives. So we should revisit Parent's third rule and improve
here.

Additional style guidelines for libcudf code include:
Additional style guidelines for libcudf code:

* Prefer "east const", placing `const` after the type. This is not
automatically enforced by `clang-format` because the option
Expand All @@ -152,15 +152,17 @@ The following guidelines apply to organizing `#include` lines.
from other RAPIDS libraries, then includes from related libraries, like `<thrust/...>`, then
includes from dependencies installed with cuDF, and then standard headers (for example
`<string>`, `<iostream>`).
* Use `<>` instead of `""` unless the header is in the same directory as the source file.
* We use clang-format for grouping and sorting headers automatically. See the
`cudf/cpp/.clang-format` file for specifics.
* Use `<>` for all includes except for internal headers that are not in the `include/cudf`
directory. In other words, if it is a cuDF internal header (e.g. in the `src` or `test`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this apply to cudf_test? That means we will need to change all test files to use #include "cudf_test/...".

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because cudf_test is a library with public headers in include/cudf_test. Therefore includes of its public headers should use <>. Moreover, there is a separate regex grouping for cudf_test in .clang-format.

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 clarification of this (and nvtext) in the guide.

directory), the path will not start with `cudf` (e.g. `#include <cudf/some_header.hpp>`) so it should use quotes. Example: `#include "io/utilities/hostdevice_vector.hpp"`.
* Tools like `clangd` often auto-insert includes when they can, but they usually get the grouping
and brackets wrong.
and brackets wrong. Correct the usage of quotes or brackets and then run clang-format to correct
the grouping.
* 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`.
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/io/parquet/error.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@

#pragma once

#include <rmm/cuda_stream_view.hpp>
#include "io/utilities/hostdevice_vector.hpp"

#include <io/utilities/hostdevice_vector.hpp>
#include <rmm/cuda_stream_view.hpp>

#include <cstdint>
#include <sstream>
Expand Down
Loading