-
Notifications
You must be signed in to change notification settings - Fork 239
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
TecPlot utility #2114
TecPlot utility #2114
Conversation
std::pair<std::size_t, std::size_t> dims(0,0); | ||
std::size_t val_count(0), val_total(0); | ||
std::size_t write_count(0); | ||
while (getline(in, line)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::getline
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
/// Splits a TecPlot file containing multiple sections/zones into seperate files | ||
int splitFile(std::ifstream& in, std::string file_name) | ||
{ | ||
std::ofstream out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is out
initialized? What is the purpose of out
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out
is initalised / switched to the next file / closed in writeTecPlotSection()
{ | ||
std::ofstream out; | ||
std::string line(""); | ||
std::string name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The created name
object is empty. It is used, for instance, in line 245 and 252, but will be initialized in line 262. Could this be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all instances ("")
std::ofstream out; | ||
std::string line(""); | ||
std::string name; | ||
std::pair<std::size_t, std::size_t> dims(0,0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dims
could be more local, it is used as a temporary variabele while parsing the zone.
val_count++; | ||
} | ||
if (dataCountError(out, name, val_count, val_total)) | ||
return -3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why -3
? It seems the method could return a bool value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different values indicate different reasons for failing. -3
is indicating an index error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend to document the magic numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put an explanation on the exit codes in the documentation. Independent of that, a detailed error-message is output in each case.
std::size_t const n_scalars (scalars.size()); | ||
while (iss >> x) | ||
{ | ||
if (i > n_scalars-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that n_scalars
is zero at this point? I'm not sure, but n_scalars - 1
is probably very large. Is this okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Barring an invalid TecPlot-file, n_scalars
must be larger than 0
, otherwise the zone
-section would contain no data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment on documentation.
If you would run clang-format on the changes before merging, it would be nice.
⏩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⏩
OpenGeoSys development has been moved to GitLab. |
Currently can