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

Include headers relative to -I #675

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

tpadioleau
Copy link
Member

@tpadioleau tpadioleau commented Nov 1, 2024

  • simplifies the logic: always relative to the root,
  • should better avoid clashes with namings like "macro.hpp" or "config.hpp" that other projects could also define.

@tpadioleau tpadioleau self-assigned this Nov 1, 2024
@tpadioleau tpadioleau marked this pull request as ready for review November 4, 2024 16:30
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.

Most of the changes are making things slightly worse. We probably should gather more documentation on how to deal with #include before committing to a choice.

#include "macros.hpp"
#include "ddc/detail/macros.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this change is sound. Using "macros.hpp" will make the compiler look for a macros.hpp file in the current directory and not to browse the whole path list of includes.

Copy link
Member

Choose a reason for hiding this comment

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

#include <KokkosFFT.hpp>
#include <Kokkos_Core.hpp>

#include "ddc/ddc.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include "ddc/ddc.hpp"
#include <ddc/ddc.hpp>

Here ddc.hpp is not in a ddc subdirectory of the current kernels directory.

Comment on lines 7 to 33
#include "ddc/ddc.hpp"
#include "ddc/kernels/splines/bsplines_non_uniform.hpp"
#include "ddc/kernels/splines/bsplines_uniform.hpp"
#include "ddc/kernels/splines/constant_extrapolation_rule.hpp"
#include "ddc/kernels/splines/deriv.hpp"
#include "ddc/kernels/splines/greville_interpolation_points.hpp"
#include "ddc/kernels/splines/integrals.hpp"
#include "ddc/kernels/splines/knot_discrete_dimension_type.hpp"
#include "ddc/kernels/splines/knots_as_interpolation_points.hpp"
#include "ddc/kernels/splines/math_tools.hpp"
#include "ddc/kernels/splines/null_extrapolation_rule.hpp"
#include "ddc/kernels/splines/periodic_extrapolation_rule.hpp"
#include "ddc/kernels/splines/spline_boundary_conditions.hpp"
#include "ddc/kernels/splines/spline_builder.hpp"
#include "ddc/kernels/splines/spline_builder_2d.hpp"
#include "ddc/kernels/splines/spline_evaluator.hpp"
#include "ddc/kernels/splines/spline_evaluator_2d.hpp"
#include "ddc/kernels/splines/splines_linear_problem.hpp"
#include "ddc/kernels/splines/splines_linear_problem_2x2_blocks.hpp"
#include "ddc/kernels/splines/splines_linear_problem_3x3_blocks.hpp"
#include "ddc/kernels/splines/splines_linear_problem_band.hpp"
#include "ddc/kernels/splines/splines_linear_problem_dense.hpp"
#include "ddc/kernels/splines/splines_linear_problem_maker.hpp"
#include "ddc/kernels/splines/splines_linear_problem_pds_band.hpp"
#include "ddc/kernels/splines/splines_linear_problem_pds_tridiag.hpp"
#include "ddc/kernels/splines/splines_linear_problem_sparse.hpp"
#include "ddc/kernels/splines/view.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

If you go this path, I think <> is more appropriate.

#include <ddc/ddc.hpp>

#include "view.hpp"
#include "ddc/ddc.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is not necessary.


#include "math_tools.hpp"
#include "view.hpp"
#include "ddc/ddc.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@tpadioleau
Copy link
Member Author

Most of the changes are making things slightly worse. We probably should gather more documentation on how to deal with #include before committing to a choice.

Well a choice was made when we started the project. Of course we can change it if there is a better choice. Here is my understanding of the choice (note that I may have again misunderstood this choice):

  • we use double quotes for internals headers to actually show they are part of the project,
  • we rely on the search path of the compiler, see CMakeLists.txt the include path is injected in the build interface of the target.

@tpadioleau tpadioleau marked this pull request as draft November 6, 2024 08:24
@tpadioleau
Copy link
Member Author

Back to draft the time we get consensus

@tpadioleau
Copy link
Member Author

tpadioleau commented Nov 28, 2024

@cedricchevalier19 What do you think if:

  1. we use double quotes for the internals of the core part
  2. for the components FFT, PDI, Splines use <ddc/ddc.hpp> and double quotes for their respective internals

Regarding 1., an issue is about the experimental directory that depends on the parent directory. An easy solution is to remove it as it is not used at all for now but it does not solve the issue on a longer term. We could also introduce a new directory core so that we would have:

ddc/
├── core
├── ddc.hpp
├── details
├── experimental
└── kernels
    ├── fft.hpp
    ├── splines
    └── splines.hpp

Let me know if you have better ideas.

@tpadioleau tpadioleau force-pushed the rework-internal-headers branch 2 times, most recently from 8d9e351 to b62dd89 Compare December 23, 2024 10:31
@tpadioleau
Copy link
Member Author

Other idea:

ddc/
├── CMakeLists.txt
├── include
│   └── ddc
│       ├── ddc.hpp
│       └── impl
│           ├── detail
│           └── some_header.hpp
└── tests

with components:

ddc_fft/
├── CMakeLists.txt
├── include
│   └── ddc
│       ├── fft.hpp
│       └── impl
│           ├── detail
│           └── some_header.hpp
└── tests

@tpadioleau tpadioleau force-pushed the rework-internal-headers branch from b62dd89 to 554a311 Compare December 26, 2024 11:08
@cedricchevalier19
Copy link
Member

Other idea:

ddc/
├── CMakeLists.txt
├── include
│   └── ddc
│       ├── ddc.hpp
│       └── impl
│           ├── detail
│           └── some_header.hpp
└── tests

with components:

ddc_fft/
├── CMakeLists.txt
├── include
│   └── ddc
│       ├── fft.hpp
│       └── impl
│           ├── detail
│           └── some_header.hpp
└── tests

I think this approach is easy to enforce if you are ok dealing with deep file hierarchies.

@tpadioleau
Copy link
Member Author

I think this approach is easy to enforce if you are ok dealing with deep file hierarchies.

Are you aware of difficulties with deep file hierarchies ?

In a such a config there would still be one file that I don't really know how to handle which is config.hpp generated by cmake in the build directory. At the build stage it should be #include <config.hpp> because it would be found by specifying the path to the build directory. But once installed we could #include "config.hpp" because it gets copied with the rest of headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants