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

refactor!: harmonize syntax & include order of headers #127

Merged
merged 23 commits into from
Mar 18, 2025

Conversation

dssgabriel
Copy link
Collaborator

Fixes #120.

Removes all instances of relative paths in favor of system include-style syntax: <KokkosComm/...>

I think we can also get rid of the KokkosComm/mpi/impl/include_mpi.hpp header?

@dssgabriel dssgabriel self-assigned this Nov 4, 2024
@dssgabriel
Copy link
Collaborator Author

Follow-up questions (may be left for another PR):

  • do we want to follow standard C++ library design guidelines and use include instead of src as the base directory (since we're a header-only library)?
  • similarly, do we want to use details subdirs/namespaces instead of impl for implementation details?

See the Beman Project — an effort to support open-source, community-driven implementations of standard C++ paper proposals — example library structure for more guidance: https://github.com/beman-project/exemplar

I understand that I may be nitpicking here, feel free to dismiss the ideas if you think they're irrelevant to KokkosComm! 🙂

@cedricchevalier19
Copy link
Member

It is not clear that we will be a header-only library forever. It might be interesting to have some precompiled parts to hide some internals in our compiling units.

As for impl, it is standard practice for kokkos projects, so I think we should keep it.

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

A simpler PR with changes focused on the tests and core files where the relative include is upward will be better suited to solve the issue.

Removes all instances of relative paths in favor of system include-style
syntax: `<KokkosComm/...>
Follow C++ Core Guidelines and Google C++ Style Guide for the order of
included headers.
Headers are included in the following order:
1. Parent header (KokkosComm is not concerned by this);
2. C system headers;
3. C++ standard library headers;
4. External library headers (priority to C-based libraries);
5. Project headers:
  - Base path with `<>` for any header that is "less deeply nested" than
the current header;
  - Relative path with `""` for headers that are at the same level or
"more deeply nested" than the current header.
6. Project implementation detail headers (same syntax rules as the prior
level).

We leave a blank line between each of the above-defined include orders.

Signed-off-by: Gabriel Dos Santos <[email protected]>
Signed-off-by: Gabriel Dos Santos <[email protected]>
@dssgabriel dssgabriel force-pushed the use-system-includes branch from b3e54a1 to 80d9d2f Compare March 10, 2025 15:48
@dssgabriel
Copy link
Collaborator Author

dssgabriel commented Mar 10, 2025

Finally coming around to finish this PR!

We now follow C++ Core Guidelines and Google C++ Style Guide for the order of included headers.

Headers are included in the following order, and separated by a blank line between each level:

  1. Associated header (as KokkosComm is--currently--a header-only library, we are not concerned by this);
  2. C system headers;
  3. C++ standard library headers;
  4. External library headers (priority is given to C-based libraries, e.g. mpi.h comes before Kokkos_Core.hpp);
  5. KokkosComm headers:
    • Base path with <> for any header that is "less deeply nested" than the current header (e.g. <KokkosComm/concepts.hpp>);
    • Relative path with "" for headers that are at the same level or "more deeply nested" than the current header.
  6. KokkosComm implementation details headers (same syntax rules as the prior level).

@dssgabriel dssgabriel changed the title fix: use system include syntax for all KokkosComm headers refactor: harmonize syntax & include order of headers Mar 10, 2025
nicoleavans
nicoleavans previously approved these changes Mar 13, 2025
Copy link
Collaborator

@nicoleavans nicoleavans left a comment

Choose a reason for hiding this comment

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

Assuming mpi.hpp will be renamed and semantic blocks will be used to separate header files in KokkosComm.hpp (as already indicated in comments), I fully support these changes.

Co-authored-by: Cédric Chevalier <[email protected]>
Co-authored-by: Cédric Chevalier <[email protected]>
@dssgabriel
Copy link
Collaborator Author

An important question while we are at it: should KokkosComm "export" Kokkos Core? Currently, we include Kokkos_Core.hpp in our top-level KokkosComm/KokkosComm.hpp header, and I don't think we should. We may use it internally but it shouldn't be part of what we directly expose when including KokkosComm, IMHO.

What do you think @cedricchevalier19 @cwpearson @nicoleavans ?

Order is:
1. Main MPI CommSpace header;
2. Structures that need to be exposed (handles, requests, channels
soon);
3. Point-to-point functions;
4. Collectives;
5. Utils (only Barrier for now but think Test, Wait, etc.).

Signed-off-by: Gabriel Dos Santos <[email protected]>
@dssgabriel dssgabriel changed the title refactor: harmonize syntax & include order of headers refactor!: harmonize syntax & include order of headers Mar 13, 2025
@cedricchevalier19
Copy link
Member

An important question while we are at it: should KokkosComm "export" Kokkos Core? Currently, we include Kokkos_Core.hpp in our top-level KokkosComm/KokkosComm.hpp header, and I don't think we should. We may use it internally but it shouldn't be part of what we directly expose when including KokkosComm, IMHO.

It is a very good point. Ideally we should not export Kokkos Core, but if we do not do this we are not self usable anymore.

I think we can try to get rid of it and see if it is too annoying for our prospective users.

@dssgabriel
Copy link
Collaborator Author

Once merged, closes #119 and #114 as well.

@cwpearson
Copy link
Collaborator

Thanks @dssgabriel! You also raise some good questions. I don't think we can practically avoid exporting Kokkos_Core.hpp because we're going to have some concepts as part of our interface that are specialized on Kokkos::View.

@dssgabriel
Copy link
Collaborator Author

@cwpearson

we're going to have some concepts as part of our interface that are specialized on Kokkos::View

Isn't it sufficient to export Kokkos_Core.hpp within our headers (e.g. KokkosComm/concepts.hpp) but not explicitly in our main KokkosComm/KokkosComm.hpp entry point?

Since tests are passing without it in our main header, I think we can merge it like this and revert back to including it if that's too annoying for our users.

@cedricchevalier19 cedricchevalier19 merged commit 7c3f7d7 into kokkos:develop Mar 18, 2025
9 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
5 participants