P4C Options cleanup (#4790)

* P4C options refactoring.

Signed-off-by: fruffy <[email protected]>

* Review comments

Signed-off-by: fruffy <[email protected]>

---------

Signed-off-by: fruffy <[email protected]>

backends/p4tools/common/options.h

@@ -46,9 +46,7 @@ class AbstractP4cToolOptions : protected Util::Options {
     /// Hook for customizing options processing.
     std::vector<const char *> *process(int argc, char *const argv[]) override;
 
-    /// Checks if parsed options make sense with respect to each-other.
-    /// @returns true if the validation was successfull and false otherwise.
-    [[nodiscard]] virtual bool validateOptions() const;
+    [[nodiscard]] bool validateOptions() const override;
 
     /// The name of the tool associated with these options.
     [[nodiscard]] const std::string &getToolName() const;

frontends/common/options.cpp

@@ -18,7 +18,7 @@ limitations under the License.
 
 #include "frontends/p4/frontend.h"
 
-CompilerOptions::CompilerOptions() : ParserOptions() {
+CompilerOptions::CompilerOptions(std::string_view defaultMessage) : ParserOptions(defaultMessage) {
     registerOption(
         "--excludeFrontendPasses", "pass1[,pass2]",
         [this](const char *arg) {
@@ -176,7 +176,7 @@ CompilerOptions::CompilerOptions() : ParserOptions() {
 
 bool CompilerOptions::enable_intrinsic_metadata_fix() { return true; }
 
-void CompilerOptions::validateOptions() const {
+bool CompilerOptions::validateOptions() const {
     if (!p4RuntimeFile.isNullOrEmpty()) {
         ::warning(ErrorType::WARN_DEPRECATED,
                   "'--p4runtime-file' and '--p4runtime-format' are deprecated, "
@@ -187,4 +187,5 @@ void CompilerOptions::validateOptions() const {
                   "'--p4runtime-entries-file' is deprecated, "
                   "consider using '--p4runtime-entries-files' instead");
     }
+    return ParserOptions::validateOptions();
 }

frontends/common/options.h

@@ -27,11 +27,10 @@ limitations under the License.
 
 class CompilerOptions : public ParserOptions {
  protected:
-    // Checks if parsed options make sense with respect to each-other.
-    void validateOptions() const override;
+    bool validateOptions() const override;
 
  public:
-    CompilerOptions();
+    explicit CompilerOptions(std::string_view defaultMessage = "Compile a P4 program");
 
     // If true, skip frontend passes whose names are contained in
     // passesToExcludeFrontend vector.

frontends/common/parser_options.cpp

@@ -43,11 +43,9 @@ limitations under the License.
 const char *p4includePath = CONFIG_PKGDATADIR "/p4include";
 const char *p4_14includePath = CONFIG_PKGDATADIR "/p4_14include";
 
-const char *ParserOptions::defaultMessage = "Compile a P4 program";
-
 using namespace P4::literals;
 
-ParserOptions::ParserOptions() : Util::Options(defaultMessage) {
+ParserOptions::ParserOptions(std::string_view defaultMessage) : Util::Options(defaultMessage) {
     registerOption(
         "--help", nullptr,
         [this](const char *) {
@@ -383,12 +381,12 @@ std::vector<const char *> *ParserOptions::process(int argc, char *const argv[])
                          exename(argv[0]));
 
     auto remainingOptions = Util::Options::process(argc, argv);
-    validateOptions();
+    if (!validateOptions()) {
+        return nullptr;
+    }
     return remainingOptions;
 }
 
-void ParserOptions::validateOptions() const {}
-
 const char *ParserOptions::getIncludePath() {
     cstring path = cstring::empty;
     // the p4c driver sets environment variables for include
@@ -537,5 +535,4 @@ bool P4CContext::isRecognizedDiagnostic(cstring diagnostic) {
 
     return recognizedDiagnostics.count(diagnostic);
 }
-
 const P4CConfiguration &P4CContext::getConfigImpl() { return DefaultP4CConfiguration::get(); }

frontends/common/parser_options.h

@@ -21,79 +21,76 @@ limitations under the License.
 
 #include <filesystem>
 #include <set>
-#include <unordered_map>
 
 #include "ir/configuration.h"
 #include "ir/pass_manager.h"
 #include "lib/compile_context.h"
 #include "lib/cstring.h"
 #include "lib/options.h"
 
-// Standard include paths for .p4 header files. The values are determined by
-// `configure`.
+/// Standard include paths for .p4 header files. The values are determined by
+/// `configure`.
 extern const char *p4includePath;
 extern const char *p4_14includePath;
 
-// Base class for compiler options.
-// This class contains the options for the front-ends.
-// Each back-end should subclass this file.
+/// Base class for compiler options.
+/// This class contains the options for the front-ends.
+/// Each back-end should subclass this file.
 class ParserOptions : public Util::Options {
     bool close_input = false;
-    static const char *defaultMessage;
 
-    // annotation names that are to be ignored by the compiler
+    /// Annotation names that are to be ignored by the compiler.
     std::set<cstring> disabledAnnotations;
 
-    // used to generate dump file names
+    /// Used to generate dump file names.
     mutable size_t dump_uid = 0;
 
  protected:
-    // Function that is returned by getDebugHook.
+    /// Function that is returned by getDebugHook.
     void dumpPass(const char *manager, unsigned seq, const char *pass, const IR::Node *node) const;
-    // Checks if parsed options make sense with respect to each-other.
-    virtual void validateOptions() const;
 
  public:
-    ParserOptions();
+    explicit ParserOptions(std::string_view defaultMessage = "Parse a P4 program");
+
     std::vector<const char *> *process(int argc, char *const argv[]) override;
     enum class FrontendVersion { P4_14, P4_16 };
-    // Name of executable that is being run.
+    /// Name of executable that is being run.
     cstring exe_name;
-    // Which language to compile
+    /// Which language to compile
     FrontendVersion langVersion = FrontendVersion::P4_16;
-    // options to pass to preprocessor
+    /// options to pass to preprocessor
     cstring preprocessor_options = cstring::empty;
-    // file to compile (- for stdin)
+    /// file to compile (- for stdin)
     std::filesystem::path file;
-    // if true preprocess only
+    /// if true preprocess only
     bool doNotCompile = false;
-    // Compiler version.
+    /// Compiler version.
     cstring compilerVersion;
-    // if true skip preprocess
+    /// if true skip preprocess
     bool doNotPreprocess = false;
-    // substrings matched against pass names
+    /// substrings matched against pass names
     std::vector<cstring> top4;
-    // debugging dumps of programs written in this folder
+    /// debugging dumps of programs written in this folder
     std::filesystem::path dumpFolder = ".";
-    // If false, optimization of callee parsers (subparsers) inlining is disabled.
+    /// If false, optimization of callee parsers (subparsers) inlining is disabled.
     bool optimizeParserInlining = false;
-    // Expect that the only remaining argument is the input file.
+    /// Expect that the only remaining argument is the input file.
     void setInputFile();
-    // Return target specific include path.
+    /// Return target specific include path.
     const char *getIncludePath() override;
-    // Returns the output of the preprocessor.
+    /// Returns the output of the preprocessor.
     FILE *preprocess();
-    // Closes the input stream returned by preprocess.
+    /// Closes the input stream returned by preprocess.
     void closePreprocessedInput(FILE *input) const;
-    // True if we are compiling a P4 v1.0 or v1.1 program
+    /// True if we are compiling a P4 v1.0 or v1.1 program
     bool isv1() const;
-    // Get a debug hook function suitable for insertion
-    // in the pass managers that are executed.
+    /// Get a debug hook function suitable for insertion
+    /// in the pass managers that are executed.
     DebugHook getDebugHook() const;
-    // Check whether this particular annotation was disabled
+    /// Check whether this particular annotation was disabled
     bool isAnnotationDisabled(const IR::Annotation *a) const;
-    // Search and set 'includePathOut' to be the first valid path from the
-    // list of possible relative paths.
+    /// Search and set 'includePathOut' to be the first valid path from the
+    /// list of possible relative paths.
     bool searchForIncludePath(const char *&includePathOut, std::vector<cstring> relativePaths,
                               const char *);
     /// If true do not generate #include statements.
@@ -192,5 +189,4 @@ class P4CContextWithOptions final : public P4CContext {
     /// Compiler options for this compilation context.
     OptionsType optionsInstance;
 };
-
 #endif /* FRONTENDS_COMMON_PARSER_OPTIONS_H_*/

lib/options.cpp

@@ -109,6 +109,12 @@ std::vector<const char *> *Util::Options::process(int argc, char *const argv[])
         }
     }
 
+    auto result = validateOptions();
+    if (!result) {
+        usage();
+        return nullptr;
+    }
+
     return &remainingOptions;
 }
 
@@ -154,3 +160,5 @@ void Util::Options::usage() {
     }
     for (auto m : additionalUsage) *outStream << m << std::endl;
 }
+
+bool Util::Options::validateOptions() const { return true; }

lib/options.h

@@ -49,7 +49,7 @@ class Options {
     };
 
     // return true if processing is successful
-    typedef std::function<bool(const char *optarg)> OptionProcessor;
+    using OptionProcessor = std::function<bool(const char *)>;
 
  protected:
     struct Option {
@@ -85,6 +85,10 @@ class Options {
     explicit Options(std::string_view message)
         : binaryName(nullptr), message(message), compileCommand("") {}
 
+    /// Checks if parsed options make sense with respect to each-other.
+    /// @returns true if the validation was successful and false otherwise.
+    [[nodiscard]] virtual bool validateOptions() const;
+
  public:
     /**
      * Process options; return list of remaining options.