Policy for naming public API functions/structs #189
Comments
mabruzzo
added a commit
to mabruzzo/grackle
that referenced
this issue
Jun 3, 2024
This change was split off from GH PR grackle-project#197. This should be a lot easier to review in isolation (and I think that it is a generally useful change in its own right). Overview -------- The objective of this PR is conceptually very simple. It introduces the following function: ```c++ int gr_initialize_field_data(grackle_field_data *my_fields); ``` The idea is to have users immediately call this function right after they initialize a new `grackle_field_data` instance - When it's called the handful of non data-field members are assigned sensible defaults. It also initializes all data-field members to NULL pointers. - this is analogous to the way that `local_initialize_chemistry_parameters` is used. Motivation ---------- While this function is not strictly necessary, I think it is a useful addition and I think we should tell people to use it (though old code won't break). A large motivation is peace of mind. I have often wished for this sort of thing. I'm always a little concerned when using a new configuration of grackle that I didn't initialize all of the required fields. I would definitely feel a little more comfortable knowing that a field that I forgot about is assigned a ``NULL`` pointer rather than some garbage data that might let the code chug along. It could also facillitate more error checking: - such as checks in the style of the ``grid_dx`` check introduced in PR grackle-project#190 (applying similar checks to `grid_rank`, `grid_dimension`, `grid_start`, and `grid_end` requires that we know their default value). - we could explicitly check that the user specified all required data-fields. We can only do this if we know that unset data-fields have a default value of `NULL`. - **From a user friendliness perspective, I think this check alone is enough to justify the function's existence.** - We could also potentially warn if unnecessary fields were specified - Plus, we could account for the fact that functions like ``calculate_pressure`` require fewer fields that ``solve_chemistry`` - we could also add checks that none of the fields are aliases (an implicit Fortran requirement). This would be much easier to implement if we knew we could just ignore fields that were set to ``NULL`` pointers We would probably need to make these checks opt-in somehow, both because we can't be absolutely certain whether a user actually invoked ``gr_initialize_field_data`` and users might not want the runtime-cost (which could be relatively large for very small grids) Naming ------ I choose to name this function based on my suggestion in grackle-project#189 that we adopt a convention that all new functions in the stable public API share a common prefix (`gr_` or `grackle_`). For the sake of proposing something concrete, I assumed that we choose the `gr_` prefix. (If we decide on a different prefix, this would be easy to change). Alternatively, if we don't want any prefix, we could name it `initialize_grackle_field_data` (I do worry a little that could introduce naming conflicts with existing functions in downstream codes)
mabruzzo
added a commit
to mabruzzo/grackle
that referenced
this issue
Jun 3, 2024
This change was split off from GH PR grackle-project#197. This should be a lot easier to review in isolation (and I think that it is a generally useful change in its own right). Overview -------- The objective of this PR is conceptually very simple. It introduces the following function: ```c++ int gr_initialize_field_data(grackle_field_data *my_fields); ``` The idea is to have users immediately call this function right after they initialize a new `grackle_field_data` instance - When it's called the handful of non data-field members are assigned sensible defaults. It also initializes all data-field members to NULL pointers. - this is analogous to the way that `local_initialize_chemistry_parameters` is used. Motivation ---------- While this function is not strictly necessary, I think it is a useful addition and I think we should tell people to use it (though old code won't break). A large motivation is peace of mind. I have often wished for this sort of thing. I'm always a little concerned when using a new configuration of grackle that I didn't initialize all of the required fields. I would definitely feel a little more comfortable knowing that a field that I forgot about is assigned a ``NULL`` pointer rather than some garbage data that might let the code chug along. It could also facillitate more error checking: - such as checks in the style of the ``grid_dx`` check introduced in PR grackle-project#190 (applying similar checks to `grid_rank`, `grid_dimension`, `grid_start`, and `grid_end` requires that we know their default value). - we could explicitly check that the user specified all required data-fields. We can only do this if we know that unset data-fields have a default value of `NULL`. - **From a user friendliness perspective, I think this check alone is enough to justify the function's existence.** - We could also potentially warn if unnecessary fields were specified - Plus, we could account for the fact that functions like ``calculate_pressure`` require fewer fields that ``solve_chemistry`` - we could also add checks that none of the fields are aliases (an implicit Fortran requirement). This would be much easier to implement if we knew we could just ignore fields that were set to ``NULL`` pointers We would probably need to make these checks opt-in somehow, both because we can't be absolutely certain whether a user actually invoked ``gr_initialize_field_data`` and users might not want the runtime-cost (which could be relatively large for very small grids) Naming ------ I choose to name this function based on my suggestion in grackle-project#189 that we adopt a convention that all new functions in the stable public API share a common prefix (`gr_` or `grackle_`). For the sake of proposing something concrete, I assumed that we choose the `gr_` prefix. (If we decide on a different prefix, this would be easy to change). Alternatively, if we don't want any prefix, we could name it `initialize_grackle_field_data` (I do worry a little that could introduce naming conflicts with existing functions in downstream codes)
|
We discussed this today at the developer meeting. We have reached a consensus on adopting the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Background
A common convention for C libraries is to use a common prefix in the names of all functions/types exported by a library.
omp_prefix (e.g.omp_get_num_threads)MPI_prefix (e.g.MPI_Send)H5prefix (e.g.H5Fopen,H5Dopen)png_prefixfftw_prefixThis convention exists because C has a global namespace and problems obviously arise if 2 or more functions share the same name. (An added benefit is that it's really easy to see when your calling an "imported" function).
Currently, Grackle does not do this.
Proposal
While I don't think this has been a problem yet, I think this is something we should consider addressing (It may make grackle easier for new codes to adopt). There are 2 parts to this proposal.
Part A
I think we should adopt a policy that all newly defined public functions and types share some standardized prefix. I think that there are 2 obvious prefixes:
gr_(as ingr_float) ORgrackle_. I favor the former because it's shorter and may make the new function names more readable.1Part B
I think it may be worth renaming at least some of the existing stuff in the public API (even if we don't adopt a standard solution). We could do it in a backwards compatible manner like renaming performed in PR #139.
In particular, I think that names of the following groups of types/functions are generic enough to cause collisions2:
code_unitsstructset_velocity_units,get_velocity_units,get_temperature_unitsfunctionsparam_name_int,param_name_double,param_name_stringFor completeness I've grouped the renaming api function/type names in order of decreasing concern (in the following spoiler tag). I don't think there's any urgency for renaming them (I could easily be convinced to do it -- but renaming more of the stuff probably means we would need to keep the deprecated versions around for a lot longer).
Spoiler warning
solve_chemistry,calculate_cooling_time,calculate_dust_temperature,calculate_gamma,calculate_pressure,calculate_temperaturechemistry_data2 andchemistry_data_storagestructs and the associated functions are of little concernchemistry_data_storagestructget_grackle_versionfunction is not a concernI definitely would appreciate input on this
Footnotes
Keep in mind that the standard only guarantees that the first 31 characters in an exported identifier are honored (mentioned here) ↩
Even if they don't cause collisions, these particular highlighted function names could definitely cause some confusion. If I were reading a downstream application for the first time, I might be surprised to learn that
get_temperature_unitsis a grackle-specific function. ↩ ↩2The text was updated successfully, but these errors were encountered: