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

kokkos: add IWYU pragmas to mark private and exported headers #6935

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

tmranse
Copy link

@tmranse tmranse commented Apr 12, 2024

This MR is an initial stab at the feature request outlined in #6933 to support IWYU pragmas. The basic approach taken here was to build up a dependency graph based on header includes, and propagate "privateness" to files that include private files without the KOKKOS_IMPL_PUBLIC_INCLUDE guard. If there is a single parent header file that is public (or the file is reachable by Kokkos_Core.hpp), suggest that as alternative in the private pragma.

There were a fair number of files that couldn't be resolved as public or private in this way so they were left untouched.

@dalg24 can you loop in relevant reviews here? and do you have any ideas for testing?

@crtrott
Copy link
Member

crtrott commented Apr 12, 2024

Thanks for the PR, I hadn't quite realized how many headers we have ....

@crtrott
Copy link
Member

crtrott commented Apr 12, 2024

Ok to test.

@dalg24
Copy link
Member

dalg24 commented Apr 12, 2024

Question about the general approach:
Why did you choose to add IWYU pragmas throughout Kokkos headers?
As I understand it, an alternative would be that we produce a kokkos.imp mapping file that can be consumed downstream. The mapping file's approach has the advantage of not being intrusive and leave up to Kokkos whether and how to cleanup its internal mess.

Comment about testing
I suppose we could introduce -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE="include-what-you-use;--error" in a number of CI builds.
It sounds like they are some clang-based tools that are compatible with IWYU that we could also run (clang-include-fixer or whatnot) that we could also run as part of our automated testing

@crtrott
Copy link
Member

crtrott commented Apr 24, 2024

We might have someone to look at this later in May again, if you don't have time to explore a file mapping approach similar to libc++.

@tmranse
Copy link
Author

tmranse commented Apr 24, 2024

Sorry I lost this thread -- We're not opposed to generating a kokkos.imp mapping if thats the route you chose to go. The reason I preferred modifying the Kokkos source directly is that Kokkos as a product has a better idea of which files it wants people including to access certain features/could update that as it changes. Instead if we are maintaining the mapping, it would be more reactive and would likely only be updated whenever we hit the impl guards. There is also the benefit that other users implicitly get the changes instead of having to have their own map.

As for testing I think either of those sounds reasonable to me. I'd likely have to defer to you folks to actually hook that up/modify this MR to do so.

@crtrott
Copy link
Member

crtrott commented Apr 27, 2024

What we meant is that the kokkos.imp file would be maintained here in Kokkos not by the end-user. We had a little experience with that in LLVM, and it feels a bit more maintainable than adding stuff to hundreds of files? What is your experience with that?

@tmranse
Copy link
Author

tmranse commented Apr 30, 2024

Ah I see -- in that case I do not have a strong preference between the IWYU pragmas and a Kokkos-maintained kokkos.imp file. I can close out this MR and create a first stab using that approach instead if you'd prefer. I agree it might be easier -- though we might want to make use of e.g. impl/* wildcards where possible so it doesn't get out of date.

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

3 participants