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

Add const specifiers to various virtual member functions #849

Merged

Conversation

program--
Copy link
Contributor

@program-- program-- commented Jul 3, 2024

This PR adds const specifiers as applicable to classes associated with DataProvider<...>.

The primary reason for this change is to ensure const-correctness within the DataProvider class hierarchy, as there's no reason not to (at least no reasons that I can think of immediately).

Changes

const qualifies the following:

data_access::DataProvider
  • get_available_variable_names
  • get_data_start_time
  • get_data_stop_time
  • record_duration
  • get_ts_index_for_time
  • is_property_sum_over_time_step
realization::Formulation
  • get_formulation_type
  • get_required_parameters
realization::Catchment_Formulation
  • get_output_header_line
realization::Bmi_Formulation
  • convert_model_time
  • get_bmi_input_variables
  • get_bmi_model_start_time_forcing_offset_s
  • get_bmi_output_variables
  • get_config_mapped_variable_name
  • get_model_current_time
  • get_model_end_time
  • get_model_type_name
  • is_bmi_input_variable
  • is_bmi_model_time_step_fixed
  • is_bmi_output_variable
  • is_model_initialized

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@@ -40,18 +40,18 @@ namespace data_access

/** Return the variables that are accessable by this data provider */
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed a typo here

Suggested change
/** Return the variables that are accessable by this data provider */
/** Return the variables that are accessible by this data provider */

Probably not suitable to fix in this PR.

@program-- program-- marked this pull request as draft July 3, 2024 18:05
@PhilMiller
Copy link
Contributor

I think is_property_sum_over_time_step should also be const

@PhilMiller
Copy link
Contributor

Re noexcept:

  • I don't think it's a good idea to start applying that qualifier without really thinking about it project-wide
  • In this particular case, I don't think we can actually apply it accurately to all implementations of these DataProvider methods. They may call into BMI routines that might fail for whatever reason. We want that exception to get thrown and potentially handled at whatever level, and not just immediately call terminate when they hit this layer of the call stack.

So, my take is that we should go ahead with adding const, and hold off on noexcept for now.

@program--
Copy link
Contributor Author

Re noexcept:

...

Yeah I agree. I forgot the extent of the DataProvider hierarchy (thankfully CI reminded me 😄), but I'm happy with only const being added.

@program-- program-- force-pushed the jsm-forcings-engine-provider-mod1 branch from 2b7fae9 to 25823b6 Compare July 3, 2024 19:03
@program-- program-- changed the title Add const and noexcept specifiers to DataProvider virtual members Add const specifiers to various virtual member functions Jul 3, 2024
@program-- program-- marked this pull request as ready for review July 3, 2024 19:34
@PhilMiller
Copy link
Contributor

This all looks good, but I'm sick enough that I'm not going to press "merge" on anything myself today.

@program-- program-- merged commit 9fc3182 into NOAA-OWP:master Jul 22, 2024
19 checks passed
@program-- program-- deleted the jsm-forcings-engine-provider-mod1 branch July 22, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants