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

Remove redundant simulation type check #363

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

MattRolchigo
Copy link
Collaborator

Not sure how this got into the intermediate output subroutine to begin with, the simulation type only needs to be checked during the input read since it doesn't change at all during a run

Copy link
Collaborator

@streeve streeve left a comment

Choose a reason for hiding this comment

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

I put it anywhere we pass the type, generally in case you weren't using the inputs directly. Is it worth it? I don't know, but there are two other places aside from the initial check during input parsing:

src/CAgrid.hpp: validSimulationType(simulation_type);
src/CAinputs.hpp: validSimulationType(simulation_type);
src/CAtemperature.hpp: validSimulationType(simulation_type);

@MattRolchigo
Copy link
Collaborator Author

MattRolchigo commented Jul 1, 2024

It's not worth it - simulation type does not change once it is set in the constructor, if it's an invalid simulation type that should be caught during initialization but it doesn't need to be checked every time. Though I see why you did it, it would theoretically be possible to accidentally pass an std::string that was not simulation_type and cause an error

Copy link
Collaborator

@streeve streeve left a comment

Choose a reason for hiding this comment

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

If there's ever a strange error because of a mistaken simulation type I will gloat

@MattRolchigo
Copy link
Collaborator Author

Fair enough, I felt good about this because the only times that we pass the problem type to a function, its the only input of type std::string so it won't get mixed up with another input for now

@MattRolchigo MattRolchigo merged commit 2606b03 into LLNL:master Jul 3, 2024
15 checks passed
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