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

Clean up code 59 #67

Merged
merged 14 commits into from
Nov 7, 2023
Merged

Clean up code 59 #67

merged 14 commits into from
Nov 7, 2023

Conversation

ktbolt
Copy link
Contributor

@ktbolt ktbolt commented Nov 7, 2023

These are some code clean cleaning changes I made for #59.

Most of these are simple changes.

I also added improved error checking in the applications main function.

@ktbolt ktbolt requested a review from mrp089 November 7, 2023 00:55
@mrp089
Copy link
Member

mrp089 commented Nov 7, 2023

@ktbolt, I think something in merge 7182a1f went wrong

@ktbolt
Copy link
Contributor Author

ktbolt commented Nov 7, 2023

@mrp089 Yes, it looks odd. Shall I close this and merge from the command line?

@mrp089
Copy link
Member

mrp089 commented Nov 7, 2023

@ktbolt, yeah, that's probably easier. I think the build error is just some variable naming.

@mrp089
Copy link
Member

mrp089 commented Nov 7, 2023

You can still keep this pull request open. Whatever you commit to this branch will automatically show up here.

Copy link
Member

@mrp089 mrp089 left a comment

Choose a reason for hiding this comment

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

@ktbolt looks good! I just had three questions for my understanding.

std::cout << "[svzerodsolver] Error: Parsing the input file '" << input_file_name << "' has failed." << std::endl;
std::cout << "[svzerodsolver] Details of the parsing error: " << std::endl;
std::cout << e.what() << std::endl;
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate all the error handling! Why do we never throw an actual error?

@@ -96,7 +96,7 @@ class SparseSystem {
*
* @param n Size of the system
*/
SparseSystem(unsigned int n);
SparseSystem(int n);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove a couple of unsigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsigned int's are really only used when doing something with bit values. If you set an unsigned int to a negative value then you get garbage.

You can argue that it can be self-documenting, that n should be >= 0. But isn't it better to actually use a better name such as system_size that documents what it is rather than n?

@@ -61,34 +61,32 @@ extern "C" void initialize(std::string input_file, int& problem_id,
std::vector<std::string>& block_names,
std::vector<std::string>& variable_names);

extern "C" void set_external_step_size(const int problem_id,
extern "C" void set_external_step_size(int problem_id,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove a couple of const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An int is passed by value so you don't need to const qualify it, doesn't make a difference to the caller.

@ktbolt ktbolt merged commit 3659f6d into SimVascular:master Nov 7, 2023
6 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