Skip to content

Conversation

mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Aug 29, 2025

this must be reviewed after #397 and #398 are merged


This converts some initialization routines to C++. This is part of the plan to transcribe lookup_rate_cool_1d

It may be easier to review this commit-by-commit (the PR is only 5 commits)

For context, the historic convention in Grackle project was to use .C as
the suffix for C++ source files. Thus, when we began transcribing files,
we used the .C suffix (even though it is a less common choice than .cpp)

However, it turns out that converting a file named `<prefix>.c` to
`<prefix>.C` causes lots of issues performing Git operations when your
machine has a case-insensitive file-system (common on macOS). This comes
up in operations as simple as changing between 2 branches

Consequently, we will be transitioning to the .cpp suffix
A few tweaks were necessary:
- I needed to add the `extern "C"` annotation to a number of functions
  that are publicly exposed as the C API
- I removed the leading underscore from some internal helper functions
- I needed to tweak make the group name argument passed to
  initialize_cloudy_data expect a `const char*`, rather than just a
  `char*`. This is essential since we are passing a string literal (this
  was a very minimal change)

I also added to the `static` specifier to some internal functions (this
ensures that these helper functions will not be accessible to external
code)
@mabruzzo mabruzzo force-pushed the ncc-initialize-chem-cpp branch from bcab799 to faddb23 Compare August 29, 2025 15:26
I also renamed initialize_rates.h to initialize_rates.hpp to denote that
the header is **ONLY** compatible with C++ source files
@mabruzzo mabruzzo force-pushed the ncc-initialize-chem-cpp branch from faddb23 to 0f75d2d Compare August 29, 2025 15:28
@mabruzzo mabruzzo moved this to Awaiting Review in New Chemistry and C++ Transcription Aug 29, 2025
@mabruzzo mabruzzo force-pushed the ncc-initialize-chem-cpp branch from 0f75d2d to 142d9de Compare August 29, 2025 15:53
@brittonsmith brittonsmith changed the base branch from newchem-cpp to main September 24, 2025 13:27
@brittonsmith brittonsmith changed the base branch from main to newchem-cpp September 24, 2025 13:27
Copy link
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Looks fine to me after conflicts resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting Review
Development

Successfully merging this pull request may close these issues.

2 participants