Include headers relative to -I #675

tpadioleau · 2024-11-01T17:26:30Z

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.

cedricchevalier19

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.

cedricchevalier19 · 2024-11-05T21:11:13Z

include/ddc/detail/kokkos.hpp

-#include "macros.hpp"
+#include "ddc/detail/macros.hpp"


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.

"source": https://blogs.stonesteps.ca/1/p/64

cedricchevalier19 · 2024-11-05T21:13:50Z

include/ddc/kernels/fft.hpp

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

+#include "ddc/ddc.hpp"


Suggested change

#include "ddc/ddc.hpp"

#include <ddc/ddc.hpp>

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

cedricchevalier19 · 2024-11-05T21:14:36Z

include/ddc/kernels/splines.hpp

+#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"


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

cedricchevalier19 · 2024-11-05T21:15:27Z

include/ddc/kernels/splines/bsplines_non_uniform.hpp

-#include <ddc/ddc.hpp>
-
-#include "view.hpp"
+#include "ddc/ddc.hpp"


I think this change is not necessary.

cedricchevalier19 · 2024-11-05T21:15:41Z

include/ddc/kernels/splines/bsplines_uniform.hpp

-
-#include "math_tools.hpp"
-#include "view.hpp"
+#include "ddc/ddc.hpp"


tpadioleau · 2024-11-06T08:24:05Z

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 · 2024-11-06T08:24:27Z

Back to draft the time we get consensus

tpadioleau · 2024-11-28T13:37:36Z

@cedricchevalier19 What do you think if:

we use double quotes for the internals of the core part
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 · 2024-12-23T10:45:49Z

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

cedricchevalier19 · 2025-01-08T14:02:35Z

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 · 2025-01-08T14:24:17Z

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.

tpadioleau · 2025-05-26T19:43:53Z

The new logic is the following:

public headers are included with angle brackets, as a reminder the public headers are
- <ddc/ddc.hpp>
- <ddc/kernels/fft.hpp>
- <ddc/kernels/splines.hpp>
- <ddc/pdi.hpp>
private headers are included using quotes and relative to the file of inclusion

The only exception is about the generated header <ddc/config.hpp>.

codecov · 2025-05-26T19:50:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.03%. Comparing base (9aacefb) to head (50a1efa).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #675   +/-   ##
=======================================
  Coverage   90.03%   90.03%           
=======================================
  Files          55       55           
  Lines        2851     2851           
  Branches      969      969           
=======================================
  Hits         2567     2567           
  Misses         90       90           
  Partials      194      194

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tpadioleau self-assigned this Nov 1, 2024

tpadioleau marked this pull request as ready for review November 4, 2024 16:30

cedricchevalier19 reviewed Nov 5, 2024

View reviewed changes

tpadioleau marked this pull request as draft November 6, 2024 08:24

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

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

Quotes for private headers, angle brackets for public headers

50a1efa

tpadioleau force-pushed the rework-internal-headers branch from 554a311 to 50a1efa Compare May 26, 2025 19:33

tpadioleau marked this pull request as ready for review May 26, 2025 19:41

tpadioleau merged commit 9cbfd5b into main May 27, 2025
71 checks passed

tpadioleau deleted the rework-internal-headers branch May 27, 2025 07:04

Include headers relative to -I #675

Include headers relative to -I #675

Uh oh!

Conversation

tpadioleau commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedricchevalier19 left a comment

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tpadioleau commented Nov 6, 2024

Uh oh!

tpadioleau commented Nov 6, 2024

Uh oh!

tpadioleau commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpadioleau commented Dec 23, 2024

Uh oh!

cedricchevalier19 commented Jan 8, 2025

Uh oh!

tpadioleau commented Jan 8, 2025

Uh oh!

tpadioleau commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

tpadioleau commented Nov 1, 2024 •

edited

Loading

tpadioleau commented Nov 28, 2024 •

edited

Loading

tpadioleau commented May 26, 2025 •

edited

Loading

codecov bot commented May 26, 2025 •

edited

Loading