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

Consolidate header naming convention #1325

Merged
merged 32 commits into from
Jul 25, 2024

Conversation

esseivaju
Copy link
Contributor

@esseivaju esseivaju commented Jul 16, 2024

Consolidate header names to use the .hh extension. This includes the following changes:

  • celeritas_cmake_string.h and celeritas_config.h consolidated and renamed to corecel/Config.hh
  • celeritas_sys_config.h renamed to corecel/sys/Config.hh
  • celeritas_version.h renamed to corecel/Version.hh
  • corecel/device_runtime_api.h renamed to corecel/DeviceRuntimeApi.hh
  • Use C++17 inline variable for globals in corecel/Config.hh and corecel/Version.hh`
  • Update .clang-format config to have config/version headers as a separate group and update deprecated options

The original *.h files are kept for backward compatibility with a warning message added for being deprecated.

@esseivaju esseivaju added core Software engineering infrastructure minor Minor internal changes or fixes labels Jul 16, 2024
@esseivaju esseivaju requested a review from sethrj July 16, 2024 19:27
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Renaming the version header is a pretty big breaking change. Would it be terrible to define this:

#ifdef __cplusplus
#define CELER_INLINE_CONSTEXPR inline constexpr
#else
#define CELER_INLINE_CONSTEXPR static const
#endif

Or am I really overthinking the C compatibility? Naming C++ files with .h is one of my many pet peeves I guess...

@esseivaju
Copy link
Contributor Author

Right, this is quite a breaking change, it's good to do it before v1.0 if we ever want to do it. I suppose that checking for a C++ compiler is an option. Realistically, I can't think of any use case for C compatibility given that all our headers use C++17. I'm not an expert in C but if we go with the macro, shouldn't we still use inline instead of static:

#ifdef __cplusplus
#define CELER_INLINE_CONSTEXPR inline constexpr
#else
#define CELER_INLINE_CONSTEXPR inline const
#endif

@sethrj
Copy link
Member

sethrj commented Jul 18, 2024

OK, looking at the various scientific packages I have installed:

Extension Count
hh 3
hpp 18
h 25
hxx 1

but more important than the extension spelling itself is the fact that the spelling is almost always consistent with the other extensions in that package. So I think using .hh is a good idea under the circumstances.

So while we're breaking backward compatibility: should we install to the even more consistent corecel/Version.hh or perhaps celeritas/Version.hh (which would show up even if we're just building geocel or orange)? (We could add a helper alias celeritas_version.h for compatibility and even add a C++ guard in there. We should do this this pull request, but we could add a deprecation warning.) Should our system version configuration data get installed to corecel/sys?

src/corecel/io/BuildOutput.cc Outdated Show resolved Hide resolved
src/celeritas_cmake_strings.hh.in Outdated Show resolved Hide resolved
@esseivaju
Copy link
Contributor Author

This is a good idea to move everything to .hh if we're renaming some files, and as you suggest we can keep an alias for compatibility. I think corecel/Version.hh is better since every package depends on it, which is not the case for celeritas.

@sethrj
Copy link
Member

sethrj commented Jul 18, 2024

The downside of that is the directory doesn't confer the same obvious package ownership as "celeritas" prefix does 🤔

@esseivaju
Copy link
Contributor Author

maybe we could have celeritas/Version.hh be an alias of corecel/Version.hh?

@esseivaju esseivaju changed the title Share global variables across TU Consolidate header naming convention Jul 20, 2024
@sethrj
Copy link
Member

sethrj commented Jul 20, 2024

@esseivaju I did originally make the "cmake strings" a separate file because I wanted to reduce the recompile impact of adding new metadata, relinking against different versions, etc... can you reassure me that's not really a problem in the big picture? 😅 (I think not but just want to run it by you.)

I am not a big fan of corecel/Version.hh... couldn't it be in sys except that there's a class sys/Version.hh? Should it be CeleritasVersion.hh and/or CeleritasConfig.hh? If we do put it in a subdirectory, maybe we should think about renaming our packages for v1.0 so that it's more obvious to a downstream project what's being included.

@esseivaju
Copy link
Contributor Author

esseivaju commented Jul 21, 2024

Mmh right, we can save a bit on compile time by keeping them separated. If the user changes celeritas_build_type or celeritas_geant4_version then we don't have to recompile files including only celeritas_config.h. I unified them since most of the variables/macros were aliased in these two files (e.g. char celeritas_core_geo and #define CELERITAS_CORE_GEO CELERITAS_CORE_GEO_ORANGE but we can keep them separated and if we want later we can add a header combining them.

I don't have a strong opinion regarding the different filenames, we can move everything to sys if you prefer, but it seems less standard. Maybe add a corecel/version or corecel/config package?

@esseivaju
Copy link
Contributor Author

@sethrj Ping on this, how about something like:

  • corecel/sys/config/Version.hh
  • corecel/sys/config/BuildOptions.hh
  • corecel/sys/config/BuildOptionsStrings.hh
  • corecel/sys/config/SysConfig.hh

@sethrj
Copy link
Member

sethrj commented Jul 25, 2024

Sorry @esseivaju . I think it's ok to have the inline strings and #defines in the same file: if recompilation is that much of a problem I should start using ccache. We should still have the version in a separate file though.

I'm still conflicted about the updated paths. Maybe we should just have as you suggested :

  • corecel/Version.hh
  • corecel/Config.hh
  • corecel/DeviceRuntimeApi.hh

And in another commit how about we define a top-level celeritas.hh that will include all the API functionality that we declare "stable", including the version and config information?

src/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Looks good to me if these couple suggestions are considered. Thanks for all the tedious work that went into the updated paths.

.clang-format Outdated Show resolved Hide resolved
src/corecel/Assert.hh Outdated Show resolved Hide resolved
src/corecel/Config.hh.in Show resolved Hide resolved
src/corecel/Config.hh.in Outdated Show resolved Hide resolved
src/corecel/Version.hh.in Outdated Show resolved Hide resolved
test/celeritas/ext/GeantImporter.test.cc Outdated Show resolved Hide resolved
@sethrj sethrj enabled auto-merge (squash) July 25, 2024 16:43
@sethrj sethrj merged commit 2d76f22 into celeritas-project:develop Jul 25, 2024
29 checks passed
@esseivaju esseivaju deleted the inline-const branch July 25, 2024 17:17
sethrj added a commit to sethrj/celeritas that referenced this pull request Aug 19, 2024
Follow-on to celeritas-project#1325 . We don't need to "configure" the `.in` files since
they're now static.
sethrj added a commit that referenced this pull request Aug 19, 2024
* Fix build with conda-supplied clang

* Move config/version cmake and files to where they're used

Follow-on to #1325 . We don't need to "configure" the `.in` files since
they're now static.

* Add a note about sincospi

* Add note about where roctx comes from

* Refactor sincospi definition for improved accuracy and readability

* Provide true implementation of sincospi

* Fix configure path

* Hopefully fix GCC build error

* Fix missing HD decorator

* Expand cospi testing

* Shortcut try-compile on subsequent builds and fix support for empty prefix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Software engineering infrastructure minor Minor internal changes or fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants