File cleanup support - #58
Conversation
Signed-off-by: John Parent <john.parent@kitware.com>
82e4a85 to
1404535
Compare
Signed-off-by: John Parent <john.parent@kitware.com>
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have one request and a couple questions
| ScopedTempFile def_rename_cleanup(link_run.get_def_file()); | ||
| if (!link_run.def_file_is_temp()) { | ||
| def_rename_cleanup.Keep(); | ||
| } |
There was a problem hiding this comment.
I prefer
if (link_run.def_file_is_temp()) {
ScopedTempFile def_rename_cleanup(link_run.get_def_file());
}
Is this the only use of keep? If so, I think that definition can be removed.
There was a problem hiding this comment.
Will remove keep. I think the later comment also has implications for this, so perhaps this stanza will end up coming out entirely.
| } | ||
| def_out.close(); | ||
| this->def_file_ = rename_def; | ||
| this->def_file_is_temp_ = true; |
There was a problem hiding this comment.
Question: could this LinkerInvocation object maintain a ScopedTempFile itself, so that it is cleaned up? Or would it be cleaned up at the wrong time?
There was a problem hiding this comment.
I think there's no good reason why we couldn't have the destructor implicitly clean this. Will switch to that approach.
| // Release the handle used to capture dumpbin's output now, otherwise it | ||
| // stays open (and blocks removal of tmp_def_file below) until def_executor | ||
| // is destroyed. | ||
| this->def_executor.CleanupHandles(); |
There was a problem hiding this comment.
Question: are executors ever called multiple times, or would it make sense to do this automatically after one call to .Execute?
There was a problem hiding this comment.
They could be. They are not in practice, but running the same command multiple times may be of interest in the future, i.e. if we wanted to verify a library's exported symbols haven't changed after our operations, or similar.
2 participants
Signed-off-by: John Parent john.parent@kitware.com