Fix #421 GOMC terminates on input Checkpoint False #423
Conversation
| //Same as checkBool but doesn't exit with an error if the string doesn't | ||
| //represent a boolean value. Use for cases where we aren't sure the argument | ||
| //string should be a boolean. | ||
| bool ConfigSetup::isBool(std::string str) | ||
| { | ||
| //short circuit for long strings | ||
| if (str.length() > 5) return false; | ||
|
|
||
| // capitalize string | ||
| for(uint k = 0; k < str.length(); k++) { | ||
| str[k] = toupper(str[k]); | ||
| } | ||
|
|
||
| return (str == "ON" || str == "TRUE" || str == "YES" || str == "OFF" || str == "FALSE" || str == "NO"); | ||
| } | ||
|
|
There was a problem hiding this comment.
I dont think we need a separate function. If we expect to read bool but user specified a keyword other that we supported (yes, no, true, false, on, off), we should exit and with warning, as we do in CheckBool(string)
There was a problem hiding this comment.
If we have the following line of input
Checkpoint argument
we don't know until we check that single argument if it is a boolean or a filename. If it is a filename, we assume true and proceed. So, there isn't a clear expectation of a boolean value in this case.
There was a problem hiding this comment.
This would make GOMC act differently from what we have. For example, in all other place, we read a boolean as second term, if it was true, then we will expect a third argument (file name). This is how the logic of GOMC config parser is written. If we want to change that, then that's another story.
There was a problem hiding this comment.
What I have in mine is:
Checkpoint false
Checkpoint true filename
Checkpooint true => Error: file name is not specified
Checkpooint filename => Error: filename is not acceptable as boolean
There was a problem hiding this comment.
I'm talking about how GOMC works without the patch. If you enter
Checkpoint filename
it currently assumes true and works. If you don't want that behavior, of course your change is preferred.
There was a problem hiding this comment.
Basically the difference is
1.
Checkpoint false
Checkpoint true filename
Checkpooint true => Error: file name is not specified
Checkpooint filename => Error: filename is not acceptable as boolean
Checkpoint false
Checkpoint true filename
Checkpooint true => Error: file name is not specified
Checkpooint filename (consideres checkpoint active and uses the file name)
There was a problem hiding this comment.
I agree. Let's get some input on which option to go with. It's easy to support either option once we know.
| @@ -313,21 +334,25 @@ void ConfigSetup::Init(const char *fileName, MultiSim const*const& multisim) | |||
| in.restart.restartFromXSCFile = true; | |||
| } else if(CheckString(line[0], "Checkpoint")) { | |||
| if (line.size() == 3){ | |||
| in.files.checkpoint.defined[0] = checkBool(line[1]); | |||
| in.restart.restartFromCheckpoint = checkBool(line[1]); | |||
| in.files.checkpoint.defined[0] = in.restart.restartFromCheckpoint = checkBool(line[1]); | |||
There was a problem hiding this comment.
We also need to consider the cases where user put comments in the config file. Like:
Checkpoint false ! specifies the checkpoint
There was a problem hiding this comment.
We can use this method instead:
in.files.checkpoint.defined[0] = true;
if (line.size() >= 3) {
in.restart.restartFromCheckpoint = checkBool(line[1]);
if (multisim != NULL) {
in.files.checkpoint.name[0] = multisim->replicaInputDirectoryPath + line[2];
} else {
in.files.checkpoint.name[0] = line[2];
}
} else if (line.size() >= 2) {
in.restart.restartFromCheckpoint = checkBool(line[1]);
}
There was a problem hiding this comment.
That will terminate with an error if the user enters
Checkpoint filename
which we currently allow with an assumption of true.
It is a good idea to handle comments, but I don't want to do it in this pull request.
GOMC terminates if the config file includes the line
Checkpoint False
with the error message that the checkpoint file "False" cannot be opened.
The patch tests the input when Checkpoint has only one parameter. When it is a boolean string, like False or On, then the parameter is not interpreted as a filename. If the parameter is not a boolean string, then it is interpreted as the checkpoint filename.
The patch tests for the case and terminates if "Checkpoint True" is entered without providing the filename.