Move string parsing logic from CommandLineParser class to ppx::string_util::ParseOrDefault()

include/ppx/application.h

@@ -252,6 +252,7 @@ struct StandardOptions
 #endif
 
     std::shared_ptr<KnobFlag<std::vector<std::string>>> pAssetsPaths;
+    std::shared_ptr<KnobFlag<std::vector<std::string>>> pConfigJsonPaths;
 };
 
 // -------------------------------------------------------------------------------------------------

include/ppx/command_line_parser.h

@@ -15,6 +15,10 @@
 #ifndef ppx_command_line_parser_h
 #define ppx_command_line_parser_h
 
+#include "nlohmann/json.hpp"
+#include "ppx/log.h"
+#include "ppx/string_util.h"
+
 #include <cstdint>
 #include <ios>
 #include <optional>
@@ -34,7 +38,7 @@ namespace ppx {
 
 // All commandline flags are stored as key-value pairs (string, list of strings)
 // Value syntax:
-// - strings cannot contain "="
+// - strings cannot contain "=" or ","
 // - boolean values stored as "0" "false" "1" "true"
 //
 // GetOptionValueOrDefault() can be used to access value of specified type.
@@ -44,7 +48,7 @@ class CliOptions
 public:
     CliOptions() = default;
 
-    bool HasExtraOption(std::string_view option) const { return mAllOptions.contains(option); }
+    bool HasExtraOption(const std::string& option) const { return mAllOptions.contains(option); }
 
     // Returns the number of unique options and flags that were specified on the commandline,
     // not counting multiple appearances of the same flag such as: --assets-path a --assets-path b
@@ -55,20 +59,24 @@ class CliOptions
     // Warning: If this is called instead of the vector overload for multiple-value flags,
     //          only the last value will be returned.
     template <typename T>
-    T GetOptionValueOrDefault(std::string_view optionName, const T& defaultValue) const
+    T GetOptionValueOrDefault(const std::string& optionName, const T& defaultValue) const
     {
         auto it = mAllOptions.find(optionName);
         if (it == mAllOptions.cend()) {
             return defaultValue;
         }
         auto valueStr = it->second.back();
-        return GetParsedOrDefault<T>(valueStr, defaultValue);
+        auto result   = ppx::string_util::ParseOrDefault(valueStr, defaultValue);
+        if (result.second != std::nullopt) {
+            PPX_LOG_ERROR(result.second->errorMsg);
+        }
+        return result.first;
     }
 
     // Same as above, but intended for list flags that are specified on the command line
-    // with multiple instances of the same flag
+    // with multiple instances of the same flag, or with comma-separated values
     template <typename T>
-    std::vector<T> GetOptionValueOrDefault(std::string_view optionName, const std::vector<T>& defaultValues) const
+    std::vector<T> GetOptionValueOrDefault(const std::string& optionName, const std::vector<T>& defaultValues) const
     {
         auto it = mAllOptions.find(optionName);
         if (it == mAllOptions.cend()) {
@@ -77,21 +85,21 @@ class CliOptions
         std::vector<T> parsedValues;
         T              nullValue{};
         for (size_t i = 0; i < it->second.size(); ++i) {
-            parsedValues.emplace_back(GetParsedOrDefault<T>(it->second.at(i), nullValue));
+            auto result = ppx::string_util::ParseOrDefault(it->second.at(i), nullValue);
+            if (result.second != std::nullopt) {
+                PPX_LOG_ERROR(result.second->errorMsg);
+            }
+            parsedValues.emplace_back(result.first);
         }
         return parsedValues;
     }
 
-    // Same as above, but intended for resolution flags that are specified on command line
-    // with <Width>x<Height>
-    std::pair<int, int> GetOptionValueOrDefault(std::string_view optionName, const std::pair<int, int>& defaultValue) const;
-
     // (WILL BE DEPRECATED, USE KNOBS INSTEAD)
     // Get the parameter value after converting it into the desired integral,
     // floating-point, or boolean type. If the value fails to be converted,
     // return the specified default value.
     template <typename T>
-    T GetExtraOptionValueOrDefault(std::string_view optionName, const T& defaultValue) const
+    T GetExtraOptionValueOrDefault(const std::string& optionName, const T& defaultValue) const
     {
         static_assert(std::is_integral_v<T> || std::is_floating_point_v<T> || std::is_same_v<T, std::string>, "GetExtraOptionValueOrDefault must be called with an integral, floating-point, boolean, or std::string type");
 
@@ -101,39 +109,17 @@ class CliOptions
 private:
     // Adds new option if the option does not already exist
     // Otherwise, the new value is appended to the end of the vector of stored parameters for this option
-    void
-    AddOption(std::string_view optionName, std::string_view value);
+    void AddOption(std::string_view optionName, std::string_view value);
 
-    template <typename T>
-    T GetParsedOrDefault(std::string_view valueStr, const T& defaultValue) const
-    {
-        static_assert(std::is_integral_v<T> || std::is_floating_point_v<T> || std::is_same_v<T, std::string>, "GetParsedOrDefault must be called with an integral, floating-point, boolean, or std::string type");
-        return Parse(valueStr, defaultValue);
-    }
+    // Same as above, but appends an array of values at the same key
+    void AddOption(std::string_view optionName, const std::vector<std::string>& valueArray);
 
-    // For boolean parameters
-    //   interpreted as true: "true", 1, ""
-    //   interpreted as false: "false", 0
-    bool Parse(std::string_view valueStr, bool defaultValue) const;
-
-    template <typename T>
-    T Parse(std::string_view valueStr, const T defaultValue) const
-    {
-        if constexpr (std::is_same_v<T, std::string>) {
-            return static_cast<std::string>(valueStr);
-        }
-        std::stringstream ss{static_cast<std::string>(valueStr)};
-        T                 valueAsNum;
-        ss >> valueAsNum;
-        if (ss.fail()) {
-            return defaultValue;
-        }
-        return valueAsNum;
-    }
+    // For all options existing in newOptions, current entries in mAllOptions will be replaced by them
+    void OverwriteOptions(const CliOptions& newOptions);
 
 private:
     // All flag names (string) and parameters (vector of strings) specified on the command line
-    std::unordered_map<std::string_view, std::vector<std::string_view>> mAllOptions;
+    std::unordered_map<std::string, std::vector<std::string>> mAllOptions;
 
     friend class CommandLineParser;
 };
@@ -144,26 +130,28 @@ class CliOptions
 class CommandLineParser
 {
 public:
-    struct ParsingError
-    {
-        ParsingError(const std::string& error)
-            : errorMsg(error) {}
-        std::string errorMsg;
-    };
-
     // Parse the given arguments into options. Return false if parsing
     // succeeded. Otherwise, return true if an error occurred,
     // and write the error to `out_error`.
-    std::optional<ParsingError> Parse(int argc, const char* argv[]);
+    std::optional<ppx::string_util::ParsingError> Parse(int argc, const char* argv[]);
 
+    // Parses all options specified within jsonConfig and adds them to cliOptions.
+    std::optional<ppx::string_util::ParsingError> ParseJson(CliOptions& cliOptions, const nlohmann::json& jsonConfig);
+
+    // Parses an option, handles the special --no-flag-name case, then adds the option to cliOptions
+    // Expects option names without the "--" prefix.
+    std::optional<ppx::string_util::ParsingError> ParseOption(CliOptions& cliOptions, std::string_view optionName, std::string_view valueStr);
+
+    std::string       GetJsonConfigFlagName() const { return mJsonConfigFlagName; }
     const CliOptions& GetOptions() const { return mOpts; }
     std::string       GetUsageMsg() const { return mUsageMsg; }
-    void              AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; }
 
-private:
-    CliOptions mOpts;
+    void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; }
 
-    std::string mUsageMsg = R"(
+private:
+    CliOptions  mOpts;
+    std::string mJsonConfigFlagName = "config-json-path";
+    std::string mUsageMsg           = R"(
 USAGE
 ==============================
 Boolean options can be turned on with:

include/ppx/string_util.h

@@ -23,6 +23,10 @@
 namespace ppx {
 namespace string_util {
 
+// -------------------------------------------------------------------------------------------------
+// Misc
+// -------------------------------------------------------------------------------------------------
+
 void TrimLeft(std::string& s);
 void TrimRight(std::string& s);
 
@@ -31,10 +35,18 @@ std::string TrimCopy(const std::string& s);
 // Trims all characters specified in c from both the left and right sides of s
 std::string_view TrimBothEnds(std::string_view s, std::string_view c = " \t");
 
+// Splits s at every instance of delimeter and returns a vector of substrings
+// Returns std::nullopt if s contains: leading/trailing/consecutive delimiters
+std::optional<std::vector<std::string_view>> Split(std::string_view s, char delimiter);
+
 // Splits s at the first instance of delimeter and returns two substrings
-// Returns std::nullopt if s does not contain the delimeter
+// Returns std::nullopt if s is not in expected format of string-delimeter-string
 std::optional<std::pair<std::string_view, std::string_view>> SplitInTwo(std::string_view s, char delimiter);
 
+// -------------------------------------------------------------------------------------------------
+// Formatting Strings
+// -------------------------------------------------------------------------------------------------
+
 // Formats string for printing with the specified width and left indent.
 // Words will be pushed to the subsequent line to avoid line breaks in the
 // middle of a word if possible.
@@ -74,6 +86,82 @@ std::string ToString(std::pair<T, T> values)
     return ss.str();
 }
 
+// -------------------------------------------------------------------------------------------------
+// Parsing Strings
+// -------------------------------------------------------------------------------------------------
+
+struct ParsingError
+{
+    ParsingError(const std::string& error)
+        : errorMsg(error) {}
+    std::string errorMsg;
+};
+
+// ParseOrDefault() attempts to parse valueStr into the same type as defaultValue
+// If successful, returns the parsed value and std::nullopt
+// If unsucessful, returns defaultValue and ParsingError
+
+// For strings
+// e.g. "a string" -> "a string"
+std::pair<std::string, std::optional<ParsingError>> ParseOrDefault(std::string_view valueStr, const std::string& defaultValue);
+std::pair<std::string, std::optional<ParsingError>> ParseOrDefault(std::string_view valueStr, std::string_view defaultValue);
+
+// For bool
+// e.g. "true", "1", "" -> true
+// e.g. "false", "0" -> false
+std::pair<bool, std::optional<ParsingError>> ParseOrDefault(std::string_view valueStr, bool defaultValue);
+
+// For integers, chars and floats
+// e.g. "1.0" -> 1.0f
+// e.g. "-20" -> -20
+// e.g. "c" -> 'c'
+template <typename T>
+std::pair<T, std::optional<ParsingError>> ParseOrDefault(std::string_view valueStr, T defaultValue)
+{
+    static_assert(std::is_integral_v<T> || std::is_floating_point_v<T>, "Attempted to parse invalid type for ParseOrDefault");
+
+    std::stringstream ss((std::string(valueStr)));
+    T                 valueAsNum;
+    ss >> valueAsNum;
+    if (ss.fail()) {
+        return std::make_pair(defaultValue, "could not be parsed as integral or float: " + std::string(valueStr));
+    }
+    return std::make_pair(valueAsNum, std::nullopt);
+}
+
+// For lists with comma-separated string representation
+// e.g. "i1,i2,i3 with spaces,i4" -> {"i1", "i2", "i3 with spaces", "i4"}
+template <typename T>
+std::pair<typename std::vector<T>, std::optional<ParsingError>> ParseOrDefault(std::string_view valueStr, const std::vector<T>& defaultValues)
+{
+    std::vector<std::string> splitStrings;
+    auto                     res = Split(valueStr, ',');
+    if (res == std::nullopt) {
+        // String contains no commas
+        splitStrings.emplace_back(valueStr);
+    }
+    else {
+        for (const auto sv : res.value()) {
+            splitStrings.emplace_back(std::string(sv));
+        }
+    }
+
+    std::vector<T> parsedValues;
+    T              nullValue{};
+    for (const auto& singleStr : splitStrings) {
+        auto res = ParseOrDefault(singleStr, nullValue);
+        if (res.second != std::nullopt) {
+            return std::make_pair(defaultValues, res.second);
+        }
+        parsedValues.emplace_back(res.first);
+    }
+    return std::make_pair(parsedValues, std::nullopt);
+}
+
+// For resolution with x-separated string representation
+// e.g. "600x800" -> (600, 800)
+std::pair<std::pair<int, int>, std::optional<ParsingError>> ParseOrDefault(std::string_view valueStr, const std::pair<int, int>& defaultValue);
+
 } // namespace string_util
 } // namespace ppx
 

src/ppx/application.cpp

@@ -806,13 +806,20 @@ void Application::SaveMetricsReportToDisk()
 void Application::InitStandardKnobs()
 {
     // Flag names in alphabetical order
-    std::vector<std::string> defaultAssetsPaths = {};
+    std::vector<std::string> defaultEmptyList = {};
     mStandardOpts.pAssetsPaths =
-        mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>("assets-path", defaultAssetsPaths);
+        mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>("assets-path", defaultEmptyList);
     mStandardOpts.pAssetsPaths->SetFlagDescription(
-        "Add a path in front of the assets search path list.");
+        "Add a path before the default assets folder in the search list.");
     mStandardOpts.pAssetsPaths->SetFlagParameters("<path>");
 
+    mStandardOpts.pConfigJsonPaths = mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList);
+    mStandardOpts.pConfigJsonPaths->SetFlagDescription(
+        "Additional commandline flags specified in a JSON file. Values specified in JSON files are "
+        "always overwritten by those specified on the command line. Between different files, the "
+        "later ones take priority.");
+    mStandardOpts.pConfigJsonPaths->SetFlagParameters("<path>");
+
     mStandardOpts.pDeterministic =
         mKnobManager.CreateKnob<KnobFlag<bool>>("deterministic", false);
     mStandardOpts.pDeterministic->SetFlagDescription(