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

Explicitly include <ArborX_DetailsUtils.hpp> from <ArborX.hpp> #1013

Merged
merged 2 commits into from Jan 9, 2024

Conversation

dalg24
Copy link
Contributor

@dalg24 dalg24 commented Jan 3, 2024

Rational: backward compatibility

Andrey did too good of a job at cleaning up unnecessary header includes and this led to the current situation where <ArborX_DetailsUtils.hpp> is not included in the <ArborX.hpp> header when MPI is not enabled which broke Cabana.

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

Good find, didn't realize that. Guess we don't include ArborX_DBSCAN.hpp either.

@aprokop
Copy link
Contributor

aprokop commented Jan 3, 2024

@dalg24 I've been playing around with iwyu, and was trying to figure out what files to have in ArborX.hpp:

#ifndef ARBORX_HPP
#define ARBORX_HPP

#include <ArborX_Config.hpp> // IWYU pragma: export

#include <ArborX_AccessTraits.hpp>       // IWYU pragma: export
#include <ArborX_Box.hpp>                // IWYU pragma: export
#include <ArborX_BruteForce.hpp>         // IWYU pragma: export
#include <ArborX_Callbacks.hpp>          // IWYU pragma: export
#include <ArborX_CrsGraphWrapper.hpp>    // IWYU pragma: export
#include <ArborX_Exception.hpp>          // IWYU pragma: export
#include <ArborX_GeometryTraits.hpp>     // IWYU pragma: export
#include <ArborX_HyperBox.hpp>           // IWYU pragma: export
#include <ArborX_HyperPoint.hpp>         // IWYU pragma: export
#include <ArborX_HyperSphere.hpp>        // IWYU pragma: export
#include <ArborX_IndexableGetter.hpp>    // IWYU pragma: export
#include <ArborX_KDOP.hpp>               // IWYU pragma: export
#include <ArborX_LinearBVH.hpp>          // IWYU pragma: export
#include <ArborX_PairValueIndex.hpp>     // IWYU pragma: export
#include <ArborX_Point.hpp>              // IWYU pragma: export
#include <ArborX_Predicates.hpp>         // IWYU pragma: export
#include <ArborX_SpaceFillingCurves.hpp> // IWYU pragma: export
#include <ArborX_Sphere.hpp>             // IWYU pragma: export
#include <ArborX_TraversalPolicy.hpp>    // IWYU pragma: export

#ifdef ARBORX_ENABLE_MPI
#include <ArborX_DistributedTree.hpp> // IWYU pragma: export
#include <ArborX_PairIndexRank.hpp>   // IWYU pragma: export
#endif

#endif

I'm not sure what the rule of thumb is on this kind of thing. Should we include just high level stuff, or transitives too? What do we want users to include on their own? Should it be ArborX_Core.hpp, ArborX_Clustering.hpp, ArborX_Interpolation.hpp?

src/ArborX.hpp Outdated Show resolved Hide resolved
@dalg24
Copy link
Contributor Author

dalg24 commented Jan 9, 2024

Ignoring the HIP failure

@dalg24 dalg24 merged commit c1b73bc into arborx:master Jan 9, 2024
1 of 2 checks passed
@dalg24 dalg24 deleted the include_utils branch January 9, 2024 21:52
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.

None yet

2 participants