All/bugfix/unst 9861 xmltree memory leaks#815
Conversation
…std::string for memory management
…ogger is only deleted when owned by Dimr instance
KoenBussemakerDeltares
left a comment
KoenBussemakerDeltares
left a comment
There was a problem hiding this comment.
I left a few remarks. Nice improvements in general!
| freeLibs(); | ||
|
|
||
| log->Write(DEBUG, my_rank, "dimr shutting down normally"); | ||
| if (logIsOwned) |
There was a problem hiding this comment.
Why do you want to do this conditionally? Shutdown is happening normally even when the log is not owned.
|
|
||
| if (logIsOwned) | ||
| { | ||
| delete log; |
There was a problem hiding this comment.
Consider a std::unique_ptr to store the logger instance. You can use it to check for ownership by testing against null, and you don't have to worry about cleanup.
| for (int i = 0; i < rootXml->children.size(); i++) | ||
| { | ||
| if (strcmp(rootXml->children[i]->name, "component") == 0) | ||
| if (rootXml->children[i]->name == "component") |
There was a problem hiding this comment.
This is incorrect, == will do a pointer comparison in this case (name is a char*, and the string literal will resolve to one as well). Use strcmp or put std::string("component") on the right hand side to trigger the use of std::string's equality operator.
If you want to be bold you can try changing the name from a char to a std::string, but you might just get into more trouble.
There are many other places where this happens too, make sure you check thoroughly.
| rawWorkingDir = workingDirElement->charData.c_str(); | ||
| } | ||
| // Is workingDir a valid relative path? | ||
| char* combinedPath = new char[MAXSTRING]; |
There was a problem hiding this comment.
Consider using std::filesystem to fix this messy path handling. I also understand if you want to limit the scope of this PR, it is up to you.
| this->charData = SubstEnvVar(this->charData); | ||
| } | ||
|
|
||
| for (int i = 0; i < children.size(); i++) |
There was a problem hiding this comment.
This file is an excellent reminder why we should not write our own xml parsers
|
|
||
| this->name = new char[strlen(name) + 1]; | ||
| strcpy(this->name, name); | ||
| const std::string& ppn = (parent == NULL) ? std::string() : parent->pathname; |
There was a problem hiding this comment.
If somebody ever has to remove the const it will lead to a dangling reference. I know this is guaranteed to work in it's current form, but IMO it's better to just make it a copy for clarity sake.
| struct ParseState | ||
| { | ||
| XmlTree** curnode; | ||
| std::string charDataStr; |
There was a problem hiding this comment.
Don't use the type in the name
| (*curnode)->charData[len] = '\0'; | ||
| (*curnode)->charDataLen = len + 1; | ||
| CharDataLen = 0; | ||
| // Trim leading whitespace |
There was a problem hiding this comment.
Isn't there a nice boost function that can do this?
| remainder = (char*)""; | ||
| else | ||
| *remainder++ = '\0'; | ||
| std::string path = pathname; |
There was a problem hiding this comment.
Again, consider using std::filesystem for this
3 participants
What was done
Evidence of the work done
<add video/figures if applicable>
Tests
<add testcase numbers if applicable, Issue number>
Documentation
<add description of changes if applicable, Issue number>
Issue link UNST-9861