Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up bmi_cem.c by replacing repetitive hard-coded metadata with a centralized table of variable information. The key changes include introducing the VarInfo structure with an array of variable definitions, refactoring getter functions to use the lookup table, and consolidating grid and variable metadata handling.
Comments suppressed due to low confidence (1)
bmi_cem.c:309
- After setting *grid = 2 for matched variable names, the code unconditionally resets *grid to -1 and returns BMI_FAILURE. Consider returning BMI_SUCCESS immediately after setting *grid appropriately to avoid overriding the valid assignment.
*grid = -1;
1d2fb4c to
b953aa7
Compare
There was a problem hiding this comment.
Pull Request Overview
Refactors bmi_cem.c to centralize variable metadata in a VarInfo table and simplifies per-variable lookups.
- Adds
VarInfostruct andvariables[]table with name/units/type/itemsize/location - Introduces
find_variablehelper and replaces long if-else chains for type/units/itemsize/location - Leaves per-variable logic in
get_var_grid,get_var_nbytes, andget_value_ptr, still manually matching names
Comments suppressed due to low confidence (1)
bmi_cem.c:21
- [nitpick] The array name
variablesis very generic. Consider renaming it to something more descriptive likevar_info_tableorbmi_var_info_list.
const VarInfo variables[] = {
| /* Implement this: Add model-specific includes */ | ||
| #include "cem_model.h" | ||
|
|
||
|
|
There was a problem hiding this comment.
Add a brief comment above the VarInfo typedef explaining its purpose (e.g. “// VarInfo holds metadata for each BMI variable”).
| // VarInfo holds metadata for each BMI variable, including its name, units, type, size, and location. |
| type[0] = '\0'; | ||
| return BMI_FAILURE; | ||
| } | ||
| strncpy(type, var->type, BMI_MAX_UNITS_NAME); |
There was a problem hiding this comment.
The BMI_MAX_UNITS_NAME macro is intended for unit strings. Use the appropriate max‐length macro for type names (e.g. BMI_MAX_TYPE_NAME) to avoid truncation or confusion.
| strncpy(type, var->type, BMI_MAX_UNITS_NAME); | |
| strncpy(type, var->type, BMI_MAX_TYPE_NAME); |
bmi_cem.c
Outdated
| if ( | ||
| strcmp(name, "land_surface_water_sediment~bedload__mass_flow_rate") == 0 || | ||
| strcmp(name, "land_surface__elevation") == 0 || | ||
| strcmp(name, "sea_water__depth") == 0 | ||
| ) { | ||
| *grid = 2; | ||
| } else { | ||
| fprintf(stderr, "bad grid. returning %d", BMI_FAILURE); | ||
| *grid = -1; | ||
| return BMI_FAILURE; | ||
| return BMI_SUCCESS; | ||
| } | ||
| return BMI_SUCCESS; | ||
| *grid = -1; | ||
| return BMI_FAILURE; |
There was a problem hiding this comment.
The get_var_grid function still hard‐codes a subset of variable names. Consider adding a grid_id field to VarInfo and using find_variable to eliminate this manual matching and keep metadata in one place.
| return BMI_FAILURE; | ||
| } | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Similarly, get_var_nbytes duplicates name-based logic to pick size sources. You could extend VarInfo to include grid and size logic (or a callback) to avoid repeating these conditionals.
b953aa7 to
6de5fbb
Compare
As with #19, I've cleaned up the
bmi_cem.cto use a table of variables that lists variable names and metadata.