search

celeritas-project / celeritas

Celeritas is a new Monte Carlo transport code designed to accelerate scientific discovery in high energy physics by improving detector simulation throughput and energy efficiency using GPUs.
https://celeritas-project.github.io/celeritas/
Other
65 stars 35 forks source link

Consolidate header naming convention #1325

Closed esseivaju closed 4 months ago

esseivaju commented 4 months ago

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

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

esseivaju commented 4 months ago

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 commented 4 months ago

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?

esseivaju commented 4 months ago

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 commented 4 months ago

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

esseivaju commented 4 months ago

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

sethrj commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@sethrj Ping on this, how about something like:

sethrj commented 4 months ago

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 :

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?