Skip to content

Commit

Permalink
Revise granular options parsing for Environment
Browse files Browse the repository at this point in the history 
Partially addresses libqueso#569 by addressing feedback in PR
libqueso#640, specifically:

 * Keep comments in header
 * Prefer reference to pointer
 * Cleanup m_optionsObj tests for NULL (adding require !NULL in a couple places)
 * Only initialize BoostInputOptionsParser when needed
 * Take care not to have trailing whitespace when converting set of
   integer to string

Some tests still fail with Boost program options enabled.
  • Loading branch information
@briadam
briadam committed Oct 12, 2017
1 parent ccd8e90 commit 866c2f2
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 32 deletions.
50 changes: 45 additions & 5 deletions src/core/inc/Environment.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,23 @@ class BaseEnvironment {
//@{
//! Default constructor.
/*!
* initialOptionsValues can be overridden by those in
* passedOptionsInputFileName
* initialOptionsValues apply first, followed by any overrides
* present in passedOptionsInputFileName. Historically the behavior was:
* IF alternative options passed in, use them exclusively
* ELSE IF input file, parse and set options from it
* ELSE use default options
* so the input file was ignored when options object was passed in.
*/
BaseEnvironment(const char* passedOptionsInputFileName, EnvOptionsValues* initialOptionsValues);

//! Alternate constructor.
/*!
* initialOptionsValues can be overridden by those in
* passedOptionsInputFileName
* initialOptionsValues apply first, followed by any overrides
* present in passedOptionsInputFileName. Historically the behavior was:
* IF alternative options passed in, use them exclusively
* ELSE IF input file, parse and set options from it
* ELSE use default options
* so the input file was ignored when options object was passed in.
*/
BaseEnvironment(const std::string & passedOptionsInputFileName,
EnvOptionsValues* initialOptionsValues);
Expand Down Expand Up @@ -493,10 +501,26 @@ class FullEnvironment : public BaseEnvironment {
* Initializes the full communicator, reads the options, deals with multiple
* sub-environments, e.g. dealing with sub/self/inter0-communicators, handles
* path for output files.
*
* initialOptionsValues apply first, followed by any overrides
* present in passedOptionsInputFileName. Historically the behavior was:
* IF alternative options passed in, use them exclusively
* ELSE IF input file, parse and set options from it
* ELSE use default options
* so the input file was ignored when options object was passed in.
*/
#ifdef QUESO_HAS_MPI
FullEnvironment(RawType_MPI_Comm inputComm, const char* passedOptionsInputFileName, const char* prefix, EnvOptionsValues* initialOptionsValues);

//! Parallel constructor.
/*!
* initialOptionsValues apply first, followed by any overrides
* present in passedOptionsInputFileName. Historically the behavior was:
* IF alternative options passed in, use them exclusively
* ELSE IF input file, parse and set options from it
* ELSE use default options
* so the input file was ignored when options object was passed in.
*/
FullEnvironment(RawType_MPI_Comm inputComm,
const std::string& passedOptionsInputFileName,
const std::string& prefix,
Expand All @@ -507,9 +531,25 @@ class FullEnvironment : public BaseEnvironment {
/*!
* No communicator is passed. Output path handling is exactly as in the
* parallel ctor.
*
* initialOptionsValues apply first, followed by any overrides
* present in passedOptionsInputFileName. Historically the behavior was:
* IF alternative options passed in, use them exclusively
* ELSE IF input file, parse and set options from it
* ELSE use default options
* so the input file was ignored when options object was passed in.
*/
FullEnvironment(const char* passedOptionsInputFileName, const char* prefix, EnvOptionsValues* initialOptionsValues);

//! Serial constructor.
/*!
* initialOptionsValues apply first, followed by any overrides
* present in passedOptionsInputFileName. Historically the behavior was:
* IF alternative options passed in, use them exclusively
* ELSE IF input file, parse and set options from it
* ELSE use default options
* so the input file was ignored when options object was passed in.
*/
FullEnvironment(const std::string& passedOptionsInputFileName,
const std::string& prefix,
EnvOptionsValues* initialOptionsValues);
Expand All @@ -535,7 +575,7 @@ class FullEnvironment : public BaseEnvironment {
void construct(const char *prefix);

//! Checks the options input file and reads the options.
void readOptionsInputFile(const char* prefix);
void readOptionsInputFile(const std::string& prefix);
//void queso_terminate_handler();

};
Expand Down
4 changes: 2 additions & 2 deletions src/core/inc/EnvironmentOptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,13 @@ class EnvOptionsValues
EnvOptionsValues(const EnvOptionsValues& src);

//! Set parameter option names to begin with prefix
void set_prefix(const char* prefix);
void set_prefix(const std::string& prefix);

//! Set default values for parameter options
void set_defaults();

//! Given prefix, read the input file for parameters named "prefix"+*
void parse(const BaseEnvironment* env, const char* prefix);
void parse(const BaseEnvironment& env, const std::string& prefix);

//! Destructor
virtual ~EnvOptionsValues();
Expand Down
18 changes: 6 additions & 12 deletions src/core/src/Environment.C
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,7 @@ BaseEnvironment::subDisplayFile() const
std::string
BaseEnvironment::subDisplayFileName() const
{
if (m_optionsObj == NULL) return ".";

queso_require_msg(m_optionsObj, "m_optionsObj variable is NULL");
return m_optionsObj->m_subDisplayFileName;
}
//-------------------------------------------------------
Expand Down Expand Up @@ -511,18 +510,21 @@ BaseEnvironment::basicPdfs() const
std::string
BaseEnvironment::platformName() const
{
queso_require_msg(m_optionsObj, "m_optionsObj variable is NULL");
return m_optionsObj->m_platformName;
}
//-------------------------------------------------------
std::string
BaseEnvironment::identifyingString() const
{
queso_require_msg(m_optionsObj, "m_optionsObj variable is NULL");
return m_optionsObj->m_identifyingString;
}
//-------------------------------------------------------
void
BaseEnvironment::resetIdentifyingString(const std::string& newString)
{
queso_require_msg(m_optionsObj, "m_optionsObj variable is NULL");
m_optionsObj->m_identifyingString = newString;
return;
}
Expand Down Expand Up @@ -1758,16 +1760,8 @@ void queso_terminate_handler()
}


//-------------------------------------------------------
// Previous behavior:
// * IF alternative options passed in, use them exclusively
// * ELSE IF input file, parse and set options from it
// * ELSE use default options
// New behavior:
// * START with default options or passed alternative options
// * IF input file, any parsed options override stored values
void
FullEnvironment::readOptionsInputFile(const char* prefix)
FullEnvironment::readOptionsInputFile(const std::string& prefix)
{
// Check file for readability for both Boost and GetPot cases
std::ifstream* ifs = new std::ifstream(m_optionsInputFileName.c_str());
Expand Down Expand Up @@ -1799,7 +1793,7 @@ FullEnvironment::readOptionsInputFile(const char* prefix)
m_input->parse_input_file(m_optionsInputFileName);

// allow input file options to override current options class values
m_optionsObj->parse(this, prefix);
m_optionsObj->parse(*this, prefix);

return;
}
Expand Down
30 changes: 17 additions & 13 deletions src/core/src/EnvironmentOptions.C
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ EnvOptionsValues::EnvOptionsValues()
:
m_env(NULL)
#ifndef QUESO_DISABLE_BOOST_PROGRAM_OPTIONS
, m_parser(new BoostInputOptionsParser())
, m_parser()
#endif // QUESO_DISABLE_BOOST_PROGRAM_OPTIONS
{
this->set_defaults();
Expand All @@ -60,7 +60,7 @@ EnvOptionsValues::EnvOptionsValues(const BaseEnvironment* env,
const char* prefix)
{
this->set_defaults();
this->parse(env, prefix);
this->parse(*env, prefix);
}


Expand Down Expand Up @@ -172,9 +172,9 @@ EnvOptionsValues::set_defaults()


void
EnvOptionsValues::set_prefix(const char* prefix)
EnvOptionsValues::set_prefix(const std::string& prefix)
{
m_prefix = (std::string)(prefix) + "env_";
m_prefix = prefix + "env_";

m_option_help = m_prefix + "help";
m_option_numSubEnvironments = m_prefix + "numSubEnvironments";
Expand All @@ -194,9 +194,9 @@ EnvOptionsValues::set_prefix(const char* prefix)


void
EnvOptionsValues::parse(const BaseEnvironment* env, const char* prefix)
EnvOptionsValues::parse(const BaseEnvironment& env, const std::string& prefix)
{
m_env = env;
m_env = &env;

if (m_env->optionsInputFileName().empty()) {
queso_error_msg("Missing input file is required");
Expand All @@ -206,7 +206,7 @@ EnvOptionsValues::parse(const BaseEnvironment* env, const char* prefix)

#ifndef QUESO_DISABLE_BOOST_PROGRAM_OPTIONS

m_parser.reset(new BoostInputOptionsParser(env->optionsInputFileName()));
m_parser.reset(new BoostInputOptionsParser(env.optionsInputFileName()));

// Register all options with parser
m_parser->registerOption<std::string>
Expand All @@ -225,13 +225,17 @@ EnvOptionsValues::parse(const BaseEnvironment* env, const char* prefix)
(m_option_subDisplayAllowInter0, m_subDisplayAllowInter0,
"Allow all inter0 nodes to write to output file");

// convert the current member set of integers to a string for default handling
std::ostringstream sdas_as_string;
std::ostream_iterator<unsigned int> out_it(sdas_as_string, " ");
std::copy(m_subDisplayAllowedSet.begin(), m_subDisplayAllowedSet.end(),
out_it);
// convert the current member set of integers to a string for
// default handling, omitting trailing whitespace
std::ostringstream sdas_oss;
std::set<unsigned int>::const_iterator sdas_it = m_subDisplayAllowedSet.begin();
for ( ; sdas_it != m_subDisplayAllowedSet.end(); ++sdas_it) {
if (sdas_it != m_subDisplayAllowedSet.begin())
sdas_oss << ' ';
sdas_oss << *sdas_it;
}
m_parser->registerOption<std::string>
(m_option_subDisplayAllowedSet, sdas_as_string.str(),
(m_option_subDisplayAllowedSet, sdas_oss.str(),
"subEnvs that will write to output file");

m_parser->registerOption<unsigned int>
Expand Down

0 comments on commit 866c2f2

Please sign in to comment.