Skip to content

Commit c04cd30

Browse files
adamkewleyadamkewley
authored
Merge pull request #2872 from adamkewley/feature-roll-out-cwdchanger
Replaced uses of chDir with IO::CwdChanger (RAII safety)
2 parents f6189e4 + aec64dd commit c04cd30

File tree

13 files changed

+159
-208
lines changed

13 files changed

+159
-208
lines changed

‎OpenSim/Common/IO.cpp‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -730,6 +730,21 @@ IO::CwdChanger::CwdChanger(IO::CwdChanger&& tmp) :
730730
std::swap(this->_existingDir, tmp._existingDir);
731731
}
732732

733+
IO::CwdChanger& IO::CwdChanger::operator=(CwdChanger&& tmp) {
734+
this->_existingDir.clear();
735+
std::swap(this->_existingDir, tmp._existingDir);
736+
return *this;
737+
}
738+
739+
void IO::CwdChanger::restore() {
740+
chDir(_existingDir);
741+
_existingDir.clear();
742+
}
743+
744+
void IO::CwdChanger::stay() noexcept {
745+
_existingDir.clear();
746+
}
747+
733748
IO::CwdChanger::~CwdChanger() noexcept {
734749
if (!_existingDir.empty()) {
735750
chDir(_existingDir);

‎OpenSim/Common/IO.h‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ class OSIMCOMMON_API IO {
143143
* - On destruction: switches the calling process's working directory
144144
* back to its original directory.
145145
*/
146-
class CwdChanger final {
146+
class OSIMCOMMON_API CwdChanger final {
147147
std::string _existingDir;
148148

149149
/**
@@ -184,7 +184,25 @@ class OSIMCOMMON_API IO {
184184
CwdChanger(const CwdChanger&) = delete;
185185
CwdChanger(CwdChanger&& tmp);
186186
CwdChanger& operator=(const CwdChanger&) = delete;
187-
CwdChanger& operator=(CwdChanger&&) = delete;
187+
CwdChanger& operator=(CwdChanger&&);
188+
189+
/**
190+
* Prematurely change the current working directory back to its
191+
* original location.
192+
*
193+
* This is functionally equivalent to prematurely destructing the
194+
* CwdChanger. After calling CwdChanger::restore(), the now-restored
195+
* CwdChanger will become a noop instance that, when it destructs,
196+
* will not attempt to change back to the original directory.
197+
*/
198+
void restore();
199+
200+
/**
201+
* Release CwdChanger's control over the current working directory,
202+
* such that the CwdChanger instance does not attempt to change back
203+
* to its original directory on destruction.
204+
*/
205+
void stay() noexcept;
188206

189207
/**
190208
* Destructs a CwdChanger instance.

‎OpenSim/Simulation/Model/AbstractTool.cpp‎

Lines changed: 66 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -386,59 +386,44 @@ loadModel(const string &aToolSetupFileName, ForceSet *rOriginalForceSet )
386386
"No model file was specified (<model_file> element is empty) in "
387387
"the Tool's Setup file. Consider passing `false` for the "
388388
"constructor's `aLoadModel` parameter");
389-
string saveWorkingDirectory = IO::getCwd();
390-
string directoryOfSetupFile = IO::getParentDirectory(aToolSetupFileName);
391-
IO::chDir(directoryOfSetupFile);
389+
390+
auto cwd = IO::CwdChanger::changeToParentOf(aToolSetupFileName);
392391

393392
log_info("AbstractTool {} loading model {}", getName(), _modelFile);
394393

395-
Model *model = 0;
394+
auto model = std::unique_ptr<Model>{new Model{_modelFile}};
395+
model->finalizeFromProperties();
396396

397-
try {
398-
model = new Model(_modelFile);
399-
model->finalizeFromProperties();
400-
if (rOriginalForceSet!=NULL)
401-
*rOriginalForceSet = model->getForceSet();
402-
} catch(...) { // Properly restore current directory if an exception is thrown
403-
IO::chDir(saveWorkingDirectory);
404-
throw;
397+
if (rOriginalForceSet!=NULL) {
398+
*rOriginalForceSet = model->getForceSet();
405399
}
406-
_model = model;
407-
IO::chDir(saveWorkingDirectory);
400+
_model = model.release();
408401
}
409402

410403
void AbstractTool::
411404
updateModelForces(Model& model, const string &aToolSetupFileName, ForceSet *rOriginalForceSet )
412405
{
413-
string saveWorkingDirectory = IO::getCwd();
414-
string directoryOfSetupFile = IO::getParentDirectory(aToolSetupFileName);
415-
IO::chDir(directoryOfSetupFile);
406+
auto cwd = IO::CwdChanger::changeToParentOf(aToolSetupFileName);
416407

417-
try {
418-
if(rOriginalForceSet) *rOriginalForceSet = model.getForceSet();
419-
420-
// If replacing force set read in from model file, clear it here
421-
if (_replaceForceSet){
422-
// Can no longer just remove the model's forces.
423-
// If the model is connected, then the model will
424-
// maintain a list of subcomponents that refer to garbage.
425-
model.cleanup();
426-
model.updForceSet().setSize(0);
427-
}
428-
429-
// Load force set(s)
430-
for(int i=0;i<_forceSetFiles.getSize();i++) {
431-
log_info("Adding force object set from {}", _forceSetFiles[i]);
432-
ForceSet *forceSet=new ForceSet(_forceSetFiles[i], true);
433-
model.updForceSet().append(*forceSet);
434-
}
408+
if (rOriginalForceSet) {
409+
*rOriginalForceSet = model.getForceSet();
410+
}
435411

436-
} catch (...) {
437-
IO::chDir(saveWorkingDirectory);
438-
throw;
412+
// If replacing force set read in from model file, clear it here
413+
if (_replaceForceSet){
414+
// Can no longer just remove the model's forces.
415+
// If the model is connected, then the model will
416+
// maintain a list of subcomponents that refer to garbage.
417+
model.cleanup();
418+
model.updForceSet().setSize(0);
439419
}
440420

441-
IO::chDir(saveWorkingDirectory);
421+
// Load force set(s)
422+
for(int i=0; i<_forceSetFiles.getSize(); i++) {
423+
log_info("Adding force object set from {}", _forceSetFiles[i]);
424+
ForceSet *forceSet=new ForceSet(_forceSetFiles[i], true);
425+
model.updForceSet().append(*forceSet);
426+
}
442427
}
443428
//_____________________________________________________________________________
444429
/**
@@ -663,51 +648,44 @@ void AbstractTool::removeExternalLoadsFromModel()
663648
if (iter!= aNode.element_end()){
664649
string fileName="";
665650
iter->getValueAs(fileName);
666-
if (fileName!="" && fileName != "Unassigned"){
667-
string saveWorkingDirectory = IO::getCwd();
668-
string directoryOfSetupFile = IO::getParentDirectory(getDocumentFileName());
669-
IO::chDir(directoryOfSetupFile);
670-
//bool extLoadsFile=false;
671-
try {
672-
SimTK::Xml::Document doc(fileName);
673-
doc.setIndentString("\t");
674-
Xml::Element root = doc.getRootElement();
675-
if (root.getElementTag()=="OpenSimDocument"){
676-
//int curVersion = root.getRequiredAttributeValueAs<int>("Version");
677-
Xml::element_iterator rootIter(root.element_begin("ForceSet"));
678-
if (rootIter!=root.element_end()){
679-
rootIter->setElementTag("ExternalLoads");
680-
}
681-
Xml::element_iterator iter(root.element_begin("ExternalLoads"));
682-
Xml::Element extLoadsElem = *iter;
683-
684-
SimTK::Xml::element_iterator kIter = aNode.element_begin("external_loads_model_kinematics_file");
685-
if (kIter !=aNode.element_end()){
686-
string kinFileName= "";
687-
kIter->getValueAs(kinFileName);
688-
aNode.removeNode(kIter);
689-
// Make sure no node already exist
690-
Xml::element_iterator iter2(extLoadsElem.element_begin("external_loads_model_kinematics_file"));
691-
if (iter2 == extLoadsElem.element_end())
692-
iter->insertNodeAfter(iter->element_end(), Xml::Element("external_loads_model_kinematics_file", kinFileName));
693-
else
694-
iter2->setValue(kinFileName);
695-
}
696-
SimTK::Xml::element_iterator fIter = aNode.element_begin("lowpass_cutoff_frequency_for_load_kinematics");
697-
if (fIter !=aNode.element_end()){
698-
SimTK::String freq;
699-
fIter->getValueAs(freq);
700-
Xml::element_iterator iter2(extLoadsElem.element_begin("lowpass_cutoff_frequency_for_load_kinematics"));
701-
if (iter2 == extLoadsElem.element_end())
702-
iter->insertNodeAfter(iter->element_end(), Xml::Element("lowpass_cutoff_frequency_for_load_kinematics", freq));
703-
else
704-
iter2->setValue(freq);
705-
}
706-
doc.writeToFile(fileName);
651+
if (fileName!="" && fileName != "Unassigned") {
652+
auto cwd = IO::CwdChanger::changeToParentOf(getDocumentFileName());
653+
654+
SimTK::Xml::Document doc(fileName);
655+
doc.setIndentString("\t");
656+
Xml::Element root = doc.getRootElement();
657+
if (root.getElementTag()=="OpenSimDocument"){
658+
//int curVersion = root.getRequiredAttributeValueAs<int>("Version");
659+
Xml::element_iterator rootIter(root.element_begin("ForceSet"));
660+
if (rootIter!=root.element_end()){
661+
rootIter->setElementTag("ExternalLoads");
707662
}
708-
}
709-
catch(...){
710-
IO::chDir(saveWorkingDirectory);
663+
Xml::element_iterator iter(root.element_begin("ExternalLoads"));
664+
Xml::Element extLoadsElem = *iter;
665+
666+
SimTK::Xml::element_iterator kIter = aNode.element_begin("external_loads_model_kinematics_file");
667+
if (kIter !=aNode.element_end()){
668+
string kinFileName= "";
669+
kIter->getValueAs(kinFileName);
670+
aNode.removeNode(kIter);
671+
// Make sure no node already exist
672+
Xml::element_iterator iter2(extLoadsElem.element_begin("external_loads_model_kinematics_file"));
673+
if (iter2 == extLoadsElem.element_end())
674+
iter->insertNodeAfter(iter->element_end(), Xml::Element("external_loads_model_kinematics_file", kinFileName));
675+
else
676+
iter2->setValue(kinFileName);
677+
}
678+
SimTK::Xml::element_iterator fIter = aNode.element_begin("lowpass_cutoff_frequency_for_load_kinematics");
679+
if (fIter !=aNode.element_end()){
680+
SimTK::String freq;
681+
fIter->getValueAs(freq);
682+
Xml::element_iterator iter2(extLoadsElem.element_begin("lowpass_cutoff_frequency_for_load_kinematics"));
683+
if (iter2 == extLoadsElem.element_end())
684+
iter->insertNodeAfter(iter->element_end(), Xml::Element("lowpass_cutoff_frequency_for_load_kinematics", freq));
685+
else
686+
iter2->setValue(freq);
687+
}
688+
doc.writeToFile(fileName);
711689
}
712690
}
713691
}
@@ -817,14 +795,12 @@ std::string AbstractTool::createExternalLoadsFile(const std::string& oldFile,
817795
{
818796
bool oldFileValid = !(oldFile=="" || oldFile=="Unassigned");
819797

820-
std::string savedCwd;
821-
if(getDocument()) {
822-
savedCwd = IO::getCwd();
823-
IO::chDir(IO::getParentDirectory(getDocument()->getFileName()));
824-
}
825-
if (oldFileValid){
798+
auto cwd = getDocument() != nullptr
799+
? IO::CwdChanger::changeToParentOf(getDocument()->getFileName())
800+
: IO::CwdChanger::noop();
801+
802+
if (oldFileValid) {
826803
if(!ifstream(oldFile.c_str(), ios_base::in).good()) {
827-
if(getDocument()) IO::chDir(savedCwd);
828804
string msg =
829805
"Object: ERR- Could not open file " + oldFile+ ". It may not exist or you don't have permission to read it.";
830806
throw Exception(msg,__FILE__,__LINE__);
@@ -844,7 +820,6 @@ std::string AbstractTool::createExternalLoadsFile(const std::string& oldFile,
844820
for(int i=0; i<9; i++){
845821
indices[i][0]= labels.findIndex(forceLabels[i]);
846822
if (indices[i][0]==-1){ // Something went wrong, abort here
847-
if(getDocument()) IO::chDir(savedCwd);
848823
string msg =
849824
"Object: ERR- Could not find label "+forceLabels[i]+ "in file " + oldFile+ ". Aborting.";
850825
throw Exception(msg,__FILE__,__LINE__);
@@ -868,17 +843,14 @@ std::string AbstractTool::createExternalLoadsFile(const std::string& oldFile,
868843
_externalLoads.setDataFileName(oldFile);
869844
std::string newName=oldFile.substr(0, oldFile.length()-4)+".xml";
870845
_externalLoads.print(newName);
871-
if(getDocument()) IO::chDir(savedCwd);
872846
log_cout("Created ForceSet file {} to apply forces from {}.", newName, oldFile);
873847
return newName;
874848
}
875849
else {
876-
if(getDocument()) IO::chDir(savedCwd);
877850
string msg =
878851
"Object: ERR- Only one body is specified in " + oldFile+ ".";
879852
throw Exception(msg,__FILE__,__LINE__);
880853
}
881-
if(getDocument()) IO::chDir(savedCwd);
882854
}
883855

884856
std::string AbstractTool::getTimeString(const time_t& t) const {

‎OpenSim/Simulation/Model/ContactMesh.cpp‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -99,25 +99,21 @@ SimTK::ContactGeometry::TriangleMesh* ContactMesh::
9999
SimTK::PolygonalMesh mesh;
100100
std::ifstream file;
101101
assert (_model);
102-
const std::string& savedCwd = IO::getCwd();
103-
bool restoreDirectory = false;
102+
103+
auto cwd = IO::CwdChanger::noop();
104104
if ((_model->getInputFileName()!="")
105105
&& (_model->getInputFileName()!="Unassigned")) {
106-
std::string parentDirectory = IO::getParentDirectory(
107-
_model->getInputFileName());
108-
IO::chDir(parentDirectory);
109-
restoreDirectory=true;
106+
cwd = IO::CwdChanger::changeToParentOf(_model->getInputFileName());
110107
}
108+
111109
file.open(filename.c_str());
112110
if (file.fail()){
113-
if (restoreDirectory) IO::chDir(savedCwd);
114111
throw Exception("Error loading mesh file: "+filename+". "
115112
"The file should exist in same folder with model.\n "
116113
"Loading is aborted.");
117114
}
118115
file.close();
119116
mesh.loadFile(filename);
120-
if (restoreDirectory) IO::chDir(savedCwd);
121117
_decorativeGeometry.reset(new SimTK::DecorativeMesh(mesh));
122118
return new SimTK::ContactGeometry::TriangleMesh(mesh);
123119
}

‎OpenSim/Simulation/Model/ExternalLoads.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,19 +158,20 @@ void ExternalLoads::extendConnectToModel(Model& aModel)
158158
auto loadDataFromDirectoryAdjacentToFile =
159159
[this, &forceData](const std::string& filepath) {
160160
// Change working directory the ExternalLoads location
161-
std::string savedCwd = IO::getCwd();
162-
IO::chDir(IO::getParentDirectory(filepath));
161+
auto cwd = IO::CwdChanger::changeToParentOf(filepath);
163162
try {
164163
forceData = new Storage(this->_dataFileName);
165164
}
166165
catch (const std::exception &ex) {
167166
log_error("Failed to read ExternalLoads data file '{}'.",
168167
this->_dataFileName);
169-
if (this->getDocument())
170-
IO::chDir(savedCwd);
168+
if (this->getDocument()) {
169+
cwd.restore();
170+
} else {
171+
cwd.stay();
172+
}
171173
throw(ex);
172174
}
173-
IO::chDir(savedCwd);
174175
};
175176
if (_dataFileName.length() > 0) {
176177
if(IO::FileExists(_dataFileName))

‎OpenSim/Tools/AnalyzeTool.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -536,9 +536,11 @@ bool AnalyzeTool::run(bool plotting)
536536

537537
// Do the maneuver to change then restore working directory
538538
// so that the parsing code behaves properly if called from a different directory.
539-
string saveWorkingDirectory = IO::getCwd();
540-
if (getDocument()) // When the tool is created live from GUI it has no file/document association
541-
IO::chDir(IO::getParentDirectory(getDocumentFileName()));
539+
// When the tool is created live from GUI it has no file/document association
540+
auto cwd = getDocument() != nullptr
541+
? IO::CwdChanger::changeToParentOf(getDocumentFileName())
542+
: IO::CwdChanger::noop();
543+
542544
// Use the Dynamics Tool API to handle external loads instead of outdated AbstractTool
543545
/*bool externalLoads = */createExternalLoads(_externalLoadsFileName, *_model);
544546

@@ -593,7 +595,6 @@ bool AnalyzeTool::run(bool plotting)
593595
} catch (const Exception& x) {
594596
x.print(cout);
595597
completed = false;
596-
IO::chDir(saveWorkingDirectory);
597598
throw Exception(x.what(),__FILE__,__LINE__);
598599
}
599600

@@ -602,7 +603,7 @@ bool AnalyzeTool::run(bool plotting)
602603
if (completed && _printResultFiles)
603604
printResults(getName(),getResultsDir()); // this will create results directory if necessary
604605

605-
IO::chDir(saveWorkingDirectory);
606+
cwd.restore();
606607

607608
removeExternalLoadsFromModel();
608609

0 commit comments

Comments
 (0)