Ticket #1343: 0001-program_options-report-unknown-options-as-error.patch

source/App/TAppDecoder/TAppDecCfg.cpp

@@ -93 +93 @@
   ;
 
   po::setDefaults(opts);
-  const list<const Char*>& argv_unhandled = po::scanArgv(opts, argc, (const Char**) argv);
+  po::ErrorReporter err;
+  const list<const Char*>& argv_unhandled = po::scanArgv(opts, argc, (const Char**) argv, err);
 
   for (list<const Char*>::const_iterator it = argv_unhandled.begin(); it != argv_unhandled.end(); it++)
   {
@@ -106 +107 @@
     return false;
   }
 
+  if (err.is_errored)
+  {
+    /* errors have already been reported to stderr */
+    return false;
+  }
+
   m_outputColourSpaceConvert = stringToInputColourSpaceConvert(outputColourSpaceConvert, false);
   if (m_outputColourSpaceConvert>=NUMBER_INPUT_COLOUR_SPACE_CONVERSIONS)
   {

source/App/TAppEncoder/TAppEncCfg.cpp

@@ -1047 +1047 @@
     opts.addOptions()(cOSS.str(), m_GOPList[i-1], GOPEntry());
   }
   po::setDefaults(opts);
-  const list<const Char*>& argv_unhandled = po::scanArgv(opts, argc, (const Char**) argv);
+  po::ErrorReporter err;
+  const list<const Char*>& argv_unhandled = po::scanArgv(opts, argc, (const Char**) argv, err);
 
   for (list<const Char*>::const_iterator it = argv_unhandled.begin(); it != argv_unhandled.end(); it++)
   {
@@ -1061 +1062 @@
     return false;
   }
 
+  if (err.is_errored)
+  {
+    /* error report has already been printed on stderr */
+    return false;
+  }
+
   /*
    * Set any derived parameters
    */

source/Lib/TAppCommon/program_options_lite.cpp

@@ -49 +49 @@
 {
   namespace program_options_lite
   {
+    ErrorReporter default_error_reporter;
+
+    ostream& ErrorReporter::error(const string& where)
+    {
+      is_errored = 1;
+      cerr << where << " error: ";
+      return cerr;
+    }
+
+    ostream& ErrorReporter::warn(const string& where)
+    {
+      cerr << where << " warning: ";
+      return cerr;
+    }
 
     Options::~Options()
     {
@@ -96 +110 @@
       return OptionSpecific(*this);
     }
 
-    static void setOptions(Options::NamesPtrList& opt_list, const string& value)
+    static void setOptions(Options::NamesPtrList& opt_list, const string& value, ErrorReporter& error_reporter)
     {
       /* multiple options may be registered for the same name:
        *   allow each to parse value */
       for (Options::NamesPtrList::iterator it = opt_list.begin(); it != opt_list.end(); ++it)
       {
-        (*it)->opt->parse(value);
+        (*it)->opt->parse(value, error_reporter);
       }
     }
 
@@ -235 +249 @@
       }
     }
 
-    bool storePair(Options& opts, bool allow_long, bool allow_short, const string& name, const string& value)
+    struct OptionWriter
+    {
+      OptionWriter(Options& opts, ErrorReporter& err)
+      : opts(opts), error_reporter(err)
+      {}
+      virtual ~OptionWriter() {}
+
+      virtual const string where() = 0;
+
+      bool storePair(bool allow_long, bool allow_short, const string& name, const string& value);
+      bool storePair(const string& name, const string& value)
+      {
+        return storePair(true, true, name, value);
+      }
+
+      Options& opts;
+      ErrorReporter& error_reporter;
+    };
+
+    bool OptionWriter::storePair(bool allow_long, bool allow_short, const string& name, const string& value)
     {
       bool found = false;
       Options::NamesMap::iterator opt_it;
@@ -260 +293 @@
 
       if (!found)
       {
-        /* not found */
-        cerr << "Unknown option: `" << name << "' (value:`" << value << "')" << endl;
+        error_reporter.error(where())
+          << "Unknown option `" << name << "' (value:`" << value << "')\n";
         return false;
       }
 
-      setOptions((*opt_it).second, value);
+      setOptions((*opt_it).second, value, error_reporter);
       return true;
     }
 
-    bool storePair(Options& opts, const string& name, const string& value)
+    struct ArgvParser : public OptionWriter
     {
-      return storePair(opts, true, true, name, value);
-    }
+      ArgvParser(Options& opts, ErrorReporter& error_reporter)
+      : OptionWriter(opts, error_reporter)
+      {}
+
+      const string where() { return "command line"; }
+
+      unsigned parseGNU(unsigned argc, const char* argv[]);
+      unsigned parseSHORT(unsigned argc, const char* argv[]);
+    };
 
     /**
      * returns number of extra arguments consumed
      */
-    unsigned parseGNU(Options& opts, unsigned argc, const char* argv[])
+    unsigned ArgvParser::parseGNU(unsigned argc, const char* argv[])
     {
       /* gnu style long options can take the forms:
        *  --option=arg
@@ -303 +343 @@
         }
         extra_argc_consumed = 1;
 #endif
-        if(!storePair(opts, true, false, option, "1"))
+        if(!storePair(true, false, option, "1"))
         {
           return 0;
         }
@@ -312 +352 @@
       {
         /* argument occurs after option_sep */
         string val = arg.substr(arg_opt_sep + 1);
-        storePair(opts, true, false, option, val);
+        storePair(true, false, option, val);
       }
 
       return extra_argc_consumed;
     }
 
-    unsigned parseSHORT(Options& opts, unsigned argc, const char* argv[])
+    unsigned ArgvParser::parseSHORT(unsigned argc, const char* argv[])
     {
       /* short options can take the forms:
        *  --option arg
@@ -333 +373 @@
       /* xxx, need to handle case where option isn't required */
       if (argc == 1)
       {
-        cerr << "Not processing option without argument `" << option << "'" << endl;
+        error_reporter.error(where())
+          << "Not processing option `" << option << "' without argument\n";
         return 0; /* run out of argv for argument */
       }
-      storePair(opts, false, true, option, string(argv[1]));
+      storePair(false, true, option, string(argv[1]));
 
       return 1;
     }
 
     list<const char*>
-    scanArgv(Options& opts, unsigned argc, const char* argv[])
+    scanArgv(Options& opts, unsigned argc, const char* argv[], ErrorReporter& error_reporter)
     {
+      ArgvParser avp(opts, error_reporter);
+
       /* a list for anything that didn't get handled as an option */
       list<const char*> non_option_arguments;
 
@@ -365 +408 @@
         if (argv[i][1] != '-')
         {
           /* handle short (single dash) options */
-#if 0
-          i += parsePOSIX(opts, argc - i, &argv[i]);
-#else
-          i += parseSHORT(opts, argc - i, &argv[i]);
-#endif
+          i += avp.parseSHORT(argc - i, &argv[i]);
           continue;
         }
 
@@ -384 +423 @@
         }
 
         /* handle long (double dash) options */
-        i += parseGNU(opts, argc - i, &argv[i]);
+        i += avp.parseGNU(argc - i, &argv[i]);
       }
 
       return non_option_arguments;
     }
 
-    void scanLine(Options& opts, string& line)
+    struct CfgStreamParser : public OptionWriter
+    {
+      CfgStreamParser(const string& name, Options& opts, ErrorReporter& error_reporter)
+      : OptionWriter(opts, error_reporter)
+      , name(name)
+      , linenum(0)
+      {}
+
+      const string name;
+      int linenum;
+      const string where()
+      {
+        ostringstream os;
+        os << name << ":" << linenum;
+        return os.str();
+      }
+
+      void scanLine(string& line);
+      void scanStream(istream& in);
+    };
+
+    void CfgStreamParser::scanLine(string& line)
     {
       /* strip any leading whitespace */
       size_t start = line.find_first_not_of(" \t\n\r");
@@ -413 +473 @@
       if (start == string::npos)
       {
         /* error: badly formatted line */
+        error_reporter.warn(where()) << "line formatting error\n";
         return;
       }
       if (line[start] != ':')
       {
         /* error: badly formatted line */
+        error_reporter.warn(where()) << "line formatting error\n";
         return;
       }
 
@@ -426 +488 @@
       if (start == string::npos)
       {
         /* error: badly formatted line */
+        error_reporter.warn(where()) << "line formatting error\n";
         return;
       }
 
@@ -456 +519 @@
       else
       {
         /* error: no value */
+        error_reporter.warn(where()) << "no value found\n";
         return;
       }
 
       /* store the value in option */
-      storePair(opts, true, false, option, value);
+      storePair(true, false, option, value);
     }
 
-    void scanFile(Options& opts, istream& in)
+    void CfgStreamParser::scanStream(istream& in)
     {
       do
       {
+        linenum++;
         string line;
         getline(in, line);
-        scanLine(opts, line);
+        scanLine(line);
       } while(!!in);
     }
 
@@ -483 +548 @@
       }
     }
 
-    void parseConfigFile(Options& opts, const string& filename)
+    void parseConfigFile(Options& opts, const string& filename, ErrorReporter& error_reporter)
     {
       ifstream cfgstream(filename.c_str(), ifstream::in);
       if (!cfgstream)
       {
-        cerr << "Failed to open config file: `" << filename << "'" << endl;
-        exit(EXIT_FAILURE);
+        error_reporter.error(filename) << "Failed to open config file\n";
+        return;
       }
-      scanFile(opts, cfgstream);
+      CfgStreamParser csp(filename, opts, error_reporter);
+      csp.scanStream(cfgstream);
     }
 
-  }
+}
 }
 
 //! \}

source/Lib/TAppCommon/program_options_lite.h

@@ -63 +63 @@
       const char* what() const throw() { return "Option Parse Failure"; }
     };
 
+    struct ErrorReporter
+    {
+      ErrorReporter() : is_errored(0) {}
+      virtual ~ErrorReporter() {}
+      virtual std::ostream& error(const std::string& where);
+      virtual std::ostream& warn(const std::string& where);
+      bool is_errored;
+    };
+
+    extern ErrorReporter default_error_reporter;
+
     void doHelp(std::ostream& out, Options& opts, unsigned columns = 80);
-    unsigned parseGNU(Options& opts, unsigned argc, const char* argv[]);
-    unsigned parseSHORT(Options& opts, unsigned argc, const char* argv[]);
-    std::list<const char*> scanArgv(Options& opts, unsigned argc, const char* argv[]);
-    void scanLine(Options& opts, std::string& line);
-    void scanFile(Options& opts, std::istream& in);
+    std::list<const char*> scanArgv(Options& opts, unsigned argc, const char* argv[], ErrorReporter& error_reporter = default_error_reporter);
     void setDefaults(Options& opts);
-    void parseConfigFile(Options& opts, const std::string& filename);
-    bool storePair(Options& opts, const std::string& name, const std::string& value);
+    void parseConfigFile(Options& opts, const std::string& filename, ErrorReporter& error_reporter = default_error_reporter);
 
     /** OptionBase: Virtual base class for storing information relating to a
      * specific option This base class describes common elements.  Type specific
@@ -85 +91 @@
       virtual ~OptionBase() {}
 
       /* parse argument arg, to obtain a value for the option */
-      virtual void parse(const std::string& arg) = 0;
+      virtual void parse(const std::string& arg, ErrorReporter&) = 0;
       /* set the argument to the default value */
       virtual void setDefault() = 0;
 
@@ -101 +107 @@
       : OptionBase(name, desc), opt_storage(storage), opt_default_val(default_val)
       {}
 
-      void parse(const std::string& arg);
+      void parse(const std::string& arg, ErrorReporter&);
 
       void setDefault()
       {
@@ -115 +121 @@
     /* Generic parsing */
     template<typename T>
     inline void
-    Option<T>::parse(const std::string& arg)
+    Option<T>::parse(const std::string& arg, ErrorReporter&)
     {
       std::istringstream arg_ss (arg,std::istringstream::in);
       arg_ss.exceptions(std::ios::failbit);
@@ -133 +139 @@
      * first word */
     template<>
     inline void
-    Option<std::string>::parse(const std::string& arg)
+    Option<std::string>::parse(const std::string& arg, ErrorReporter&)
     {
       opt_storage = arg;
     }
@@ -141 +147 @@
     /** Option class for argument handling using a user provided function */
     struct OptionFunc : public OptionBase
     {
-      typedef void (Func)(Options&, const std::string&);
+      typedef void (Func)(Options&, const std::string&, ErrorReporter&);
 
       OptionFunc(const std::string& name, Options& parent_, Func *func_, const std::string& desc)
       : OptionBase(name, desc), parent(parent_), func(func_)
       {}
 
-      void parse(const std::string& arg)
+      void parse(const std::string& arg, ErrorReporter& error_reporter)
       {
-        func(parent, arg);
+        func(parent, arg, error_reporter);
       }
 
       void setDefault()
@@ -159 +165 @@
 
     private:
       Options& parent;
-      void (*func)(Options&, const std::string&);
+      Func* func;
     };
 
     class OptionSpecific;