Merge pull request #1346 from Idclip/ax_improvements

Various AX improvements

openvdb_ax/openvdb_ax/ast/Parse.cc

@@ -36,12 +36,15 @@ extern void axerror (openvdb::ax::ast::Tree**, char const *s) {
 }
 
 openvdb::ax::ast::Tree::ConstPtr
-openvdb::ax::ast::parse(const char* code, openvdb::ax::Logger& logger)
+openvdb::ax::ast::parse(const char* code,
+    openvdb::ax::Logger& logger)
 {
     std::lock_guard<std::mutex> lock(sInitMutex);
     axlog = &logger; // for lexer errs
     logger.setSourceCode(code);
 
+    const size_t err = logger.errors();
+
     // reset all locations
     axlloc.first_line = axlloc.last_line = 1;
     axlloc.first_column = axlloc.last_column = 1;
@@ -56,6 +59,8 @@ openvdb::ax::ast::parse(const char* code, openvdb::ax::Logger& logger)
 
     ax_delete_buffer(buffer);
 
+    if (logger.errors() > err) ptr.reset();
+
     logger.setSourceTree(ptr);
     return ptr;
 }

openvdb_ax/openvdb_ax/ast/Parse.h

@@ -23,29 +23,35 @@ namespace OPENVDB_VERSION_NAME {
 namespace ax {
 namespace ast {
 
-/// @brief  Construct an abstract syntax tree from a code snippet. If the code is
-///         not well formed, as defined by the AX grammar, this will simply return
-///         nullptr, with the logger collecting the errors.
+/// @brief   Construct an abstract syntax tree from a code snippet.
+/// @details This method parses the provided null terminated code snippet and
+///   attempts to construct a complete abstract syntax tree (AST) which can be
+///   passed to the AX Compiler. If the code is not well formed (as defined by
+///   the AX grammar) a nullptr is returned and instances of any errors
+///   encoutered are stored to the provided logger.
 /// @note   The returned AST is const as the logger uses this to determine line
-///         and column numbers of errors/warnings in later stages. If you need to
-///         modify the tree, take a copy.
+///   and column numbers of errors/warnings in later stages. If you need to
+///   modify the tree, take a copy.
 ///
-/// @return A shared pointer to a valid const AST, or nullptr if errored.
+/// @return A shared pointer to a valid const AST. Can be a nullptr on error.
+/// @todo  In the future it may be useful for ::parse to return as much of
+///   the valid AST that exists.
 ///
 /// @param code    The code to parse
 /// @param logger  The logger to collect syntax errors
 ///
 OPENVDB_AX_API openvdb::ax::ast::Tree::ConstPtr
 parse(const char* code, ax::Logger& logger);
 
-/// @brief  Construct an abstract syntax tree from a code snippet.
-///         A runtime exception will be thrown with the first syntax error.
+/// @brief   Construct an abstract syntax tree from a code snippet.
+/// @details A runtime exception will be thrown with the first syntax error.
 ///
 /// @return A shared pointer to a valid AST.
 ///
 /// @param code The code to parse
 ///
-OPENVDB_AX_API openvdb::ax::ast::Tree::Ptr parse(const char* code);
+OPENVDB_AX_API openvdb::ax::ast::Tree::Ptr
+parse(const char* code);
 
 } // namespace ast
 } // namespace ax

openvdb_ax/openvdb_ax/ax.cc

@@ -3,7 +3,6 @@
 
 #include "ax.h"
 #include "ast/AST.h"
-#include "compiler/Logger.h"
 #include "compiler/Compiler.h"
 #include "compiler/PointExecutable.h"
 #include "compiler/VolumeExecutable.h"
@@ -27,23 +26,16 @@ namespace ax {
 
 void run(const char* ax, openvdb::GridBase& grid, const AttributeBindings& bindings)
 {
-    // Construct a logger that will output errors to cerr and suppress warnings
-    openvdb::ax::Logger logger;
     // Construct a generic compiler
     openvdb::ax::Compiler compiler;
-    // Parse the provided code and produce an abstract syntax tree
-    // @note  Throws with parser errors if invalid. Parsable code does not
-    //        necessarily equate to compilable code
-    const openvdb::ax::ast::Tree::ConstPtr
-        ast = openvdb::ax::ast::parse(ax, logger);
-    if (!ast) return;
 
     if (grid.isType<points::PointDataGrid>()) {
         // Compile for Point support and produce an executable
         // @note  Throws compiler errors on invalid code. On success, returns
         //        the executable which can be used multiple times on any inputs
         const openvdb::ax::PointExecutable::Ptr exe =
-            compiler.compile<openvdb::ax::PointExecutable>(*ast, logger);
+            compiler.compile<openvdb::ax::PointExecutable>(ax);
+        assert(exe);
 
         //Set the attribute bindings
         exe->setAttributeBindings(bindings);
@@ -56,7 +48,9 @@ void run(const char* ax, openvdb::GridBase& grid, const AttributeBindings& bindi
         // @note  Throws compiler errors on invalid code. On success, returns
         //        the executable which can be used multiple times on any inputs
         const openvdb::ax::VolumeExecutable::Ptr exe =
-            compiler.compile<openvdb::ax::VolumeExecutable>(*ast, logger);
+            compiler.compile<openvdb::ax::VolumeExecutable>(ax);
+        assert(exe);
+
         // Set the attribute bindings
         exe->setAttributeBindings(bindings);
         // Execute on the provided numerical grid
@@ -78,23 +72,17 @@ void run(const char* ax, openvdb::GridPtrVec& grids, const AttributeBindings& bi
                 "a single invocation of ax::run()");
         }
     }
-    // Construct a logger that will output errors to cerr and suppress warnings
-    openvdb::ax::Logger logger;
     // Construct a generic compiler
     openvdb::ax::Compiler compiler;
-    // Parse the provided code and produce an abstract syntax tree
-    // @note  Throws with parser errors if invalid. Parsable code does not
-    //        necessarily equate to compilable code
-    const openvdb::ax::ast::Tree::ConstPtr
-        ast = openvdb::ax::ast::parse(ax, logger);
-    if (!ast) return;
 
     if (points) {
         // Compile for Point support and produce an executable
         // @note  Throws compiler errors on invalid code. On success, returns
         //        the executable which can be used multiple times on any inputs
         const openvdb::ax::PointExecutable::Ptr exe =
-            compiler.compile<openvdb::ax::PointExecutable>(*ast, logger);
+            compiler.compile<openvdb::ax::PointExecutable>(ax);
+        assert(exe);
+
         //Set the attribute bindings
         exe->setAttributeBindings(bindings);
         // Execute on the provided points individually
@@ -108,7 +96,9 @@ void run(const char* ax, openvdb::GridPtrVec& grids, const AttributeBindings& bi
         // @note  Throws compiler errors on invalid code. On success, returns
         //        the executable which can be used multiple times on any inputs
         const openvdb::ax::VolumeExecutable::Ptr exe =
-            compiler.compile<openvdb::ax::VolumeExecutable>(*ast, logger);
+            compiler.compile<openvdb::ax::VolumeExecutable>(ax);
+        assert(exe);
+
         //Set the attribute bindings
         exe->setAttributeBindings(bindings);
         // Execute on the provided volumes

openvdb_ax/openvdb_ax/codegen/ComputeGenerator.cc

@@ -138,9 +138,8 @@ bool ComputeGenerator::generate(const ast::Tree& tree)
 
     // if traverse is false, log should have error, but can error
     // without stopping traversal, so check both
-
-    const bool result = this->traverse(&tree) && !mLog.hasError();
-    if (!result) return false;
+    const size_t err = mLog.errors();
+    if (!this->traverse(&tree) || (mLog.errors() > err)) return false;
 
     // free strings at terminating blocks
 

openvdb_ax/openvdb_ax/codegen/PointComputeGenerator.cc

@@ -840,7 +840,8 @@ AttributeRegistry::Ptr PointComputeGenerator::generate(const ast::Tree& tree)
     // full code generation
     // errors can stop traversal, but dont always, so check the log
 
-    if (!this->traverse(&tree) || mLog.hasError()) return nullptr;
+    const size_t err = mLog.errors();
+    if (!this->traverse(&tree) || (mLog.errors() > err)) return nullptr;
 
     // insert free calls for any strings
 

openvdb_ax/openvdb_ax/codegen/VolumeComputeGenerator.cc

@@ -362,7 +362,8 @@ AttributeRegistry::Ptr VolumeComputeGenerator::generate(const ast::Tree& tree)
     // full code generation
     // errors can stop traversal, but dont always, so check the log
 
-    if (!this->traverse(&tree) || mLog.hasError()) return nullptr;
+    const size_t err = mLog.errors();
+    if (!this->traverse(&tree) || (mLog.errors() > err)) return nullptr;
 
     // insert free calls for any strings
 

openvdb_ax/openvdb_ax/compiler/Compiler.cc

@@ -463,14 +463,16 @@ bool initializeGlobalFunctions(const codegen::FunctionRegistry& registry,
     return count == logger.errors();
 }
 
-void verifyTypedAccesses(const ast::Tree& tree, openvdb::ax::Logger& logger)
+bool verifyTypedAccesses(const ast::Tree& tree, openvdb::ax::Logger& logger)
 {
     // verify the attributes and external variables requested in the syntax tree
     // only have a single type. Note that the executer will also throw a runtime
     // error if the same attribute is accessed with different types, but as that's
     // currently not a valid state on a PointDataGrid, error in compilation as well
     // @todo - introduce a framework for supporting custom preprocessors
 
+    const size_t errs = logger.errors();
+
     std::unordered_map<std::string, std::string> nameType;
 
     auto attributeOp =
@@ -504,6 +506,8 @@ void verifyTypedAccesses(const ast::Tree& tree, openvdb::ax::Logger& logger)
         };
 
     ast::visitNodeType<ast::ExternalVariable>(tree, externalOp);
+
+    return logger.errors() == errs;
 }
 
 inline void
@@ -682,6 +686,12 @@ Compiler::compile(const ast::Tree& tree,
         CustomData::Ptr data,
         Logger& logger)
 {
+    // @todo  Not technically necessary for volumes but does the
+    //   executer/bindings handle this?
+    if (!verifyTypedAccesses(tree, logger)) {
+        return nullptr;
+    }
+
     // initialize the module and execution engine - the latter isn't needed
     // for IR generation but we leave the creation of the TM to the EE.
 
@@ -768,8 +778,6 @@ Compiler::compile<PointExecutable>(const ast::Tree& syntaxTree,
     PointDefaultModifier modifier;
     modifier.traverse(tree.get());
 
-    verifyTypedAccesses(*tree, logger);
-
     const std::vector<std::string> functionNames {
         codegen::PointKernelBufferRange::getDefaultName(),
         codegen::PointKernelAttributeArray::getDefaultName()
@@ -787,8 +795,6 @@ Compiler::compile<VolumeExecutable>(const ast::Tree& syntaxTree,
 {
     using GenT = codegen::codegen_internal::VolumeComputeGenerator;
 
-    verifyTypedAccesses(syntaxTree, logger);
-
     const std::vector<std::string> functionNames {
         // codegen::VolumeKernelValue::getDefaultName(), // currently unused directly
         codegen::VolumeKernelBuffer::getDefaultName(),

openvdb_ax/openvdb_ax/compiler/Compiler.h

@@ -120,14 +120,16 @@ class OPENVDB_AX_API Compiler
             [&errors] (const std::string& error) {
                 errors.emplace_back(error + "\n");
             },
-            // ignore warnings
-            [] (const std::string&) {}
+            [] (const std::string&) {} // ignore warnings
         );
         const ast::Tree::ConstPtr syntaxTree = ast::parse(code.c_str(), logger);
-        typename ExecutableT::Ptr exe;
-        if (syntaxTree) {
-            exe = this->compile<ExecutableT>(*syntaxTree, logger, data);
+        if (!errors.empty()) {
+            std::ostringstream os;
+            for (const auto& e : errors) os << e << "\n";
+            OPENVDB_THROW(AXSyntaxError, os.str());
         }
+        assert(syntaxTree);
+        typename ExecutableT::Ptr exe = this->compile<ExecutableT>(*syntaxTree, logger, data);
         if (!errors.empty()) {
             std::ostringstream os;
             for (const auto& e : errors) os << e << "\n";
@@ -153,8 +155,7 @@ class OPENVDB_AX_API Compiler
             [&errors] (const std::string& error) {
                 errors.emplace_back(error + "\n");
             },
-            // ignore warnings
-            [] (const std::string&) {}
+            [] (const std::string&) {} // ignore warnings
         );
         auto exe = compile<ExecutableT>(syntaxTree, logger, data);
         if (!errors.empty()) {

openvdb_ax/openvdb_ax/compiler/Logger.cc

@@ -148,7 +148,7 @@ std::string format(const std::string& message,
 {
     std::stringstream ss;
     ss << indent;
-    if (numbered) ss << "[" << numMessage + 1 << "] ";
+    if (numbered) ss << "[" << numMessage << "] ";
     for (auto c : message) {
         ss << c;
         if (c == '\n') ss << indent;
@@ -187,19 +187,19 @@ void Logger::setSourceCode(const char* code)
 bool Logger::error(const std::string& message,
                    const Logger::CodeLocation& lineCol)
 {
-    // already exceeded the error limit
-    if (this->atErrorLimit()) return false;
+    // check if we've already exceeded the error limit
+    const bool limit = this->atErrorLimit();
+    // Always increment the error counter
+    ++mNumErrors;
+    if (limit) return false;
     mErrorOutput(format(this->getErrorPrefix() + message,
                         lineCol,
                         this->errors(),
                         this->getNumberedOutput(),
                         this->getPrintLines(),
                         this->mSettings->mIndent,
                         this->mCode.get()));
-    ++mNumErrors;
-    // now exceeds the limit
-    if (this->atErrorLimit()) return false;
-    else return true;
+    return !this->atErrorLimit();
 }
 
 bool Logger::error(const std::string& message,
@@ -215,14 +215,14 @@ bool Logger::warning(const std::string& message,
         return this->error(message + " [warning-as-error]", lineCol);
     }
     else {
+        ++mNumWarnings;
         mWarningOutput(format(this->getWarningPrefix() + message,
                               lineCol,
                               this->warnings(),
                               this->getNumberedOutput(),
                               this->getPrintLines(),
                               this->mSettings->mIndent,
                               this->mCode.get()));
-        ++mNumWarnings;
         return true;
     }
 }

openvdb_ax/openvdb_ax/compiler/Logger.h

@@ -51,6 +51,9 @@ namespace ax {
 ///   parsing, to allow resolution of code locations when they are not
 ///   explicitly available. The Logger also stores a pointer to the AST Tree
 ///   that these nodes belong to and the code used to create it.
+///
+/// @warning  The logger is not thread safe. A unique instance of the Logger
+///   should be used for unique invocations of ax pipelines.
 class OPENVDB_AX_API Logger
 {
 public:
@@ -75,29 +78,29 @@ class OPENVDB_AX_API Logger
     ///   associated code location.
     /// @param message The error message
     /// @param lineCol The line/column number of the offending code
-    /// @return true if non-fatal and can continue to capture future messages.
+    /// @return true if can continue to capture future messages.
     bool error(const std::string& message, const CodeLocation& lineCol = CodeLocation(0,0));
 
     /// @brief Log a compiler error using the offending AST node. Used in AST
     ///   traversal.
     /// @param message The error message
     /// @param node The offending AST node causing the error
-    /// @return true if non-fatal and can continue to capture future messages.
+    /// @return true if can continue to capture future messages.
     bool error(const std::string& message, const ax::ast::Node* node);
 
     /// @brief Log a compiler warning and its offending code location. If the
     ///   offending location is (0,0), the message is treated as not having an
     ///   associated code location.
     /// @param message The warning message
     /// @param lineCol The line/column number of the offending code
-    /// @return true if non-fatal and can continue to capture future messages.
+    /// @return true if can continue to capture future messages.
     bool warning(const std::string& message, const CodeLocation& lineCol = CodeLocation(0,0));
 
     /// @brief Log a compiler warning using the offending AST node. Used in AST
     ///   traversal.
     /// @param message The warning message
     /// @param node The offending AST node causing the warning
-    /// @return true if non-fatal and can continue to capture future messages.
+    /// @return true if can continue to capture future messages.
     bool warning(const std::string& message, const ax::ast::Node* node);
 
     ///
@@ -130,7 +133,9 @@ class OPENVDB_AX_API Logger
     bool getWarningsAsErrors() const;
 
     /// @brief Sets the maximum number of errors that are allowed before
-    ///   compilation should exit
+    ///   compilation should exit.
+    /// @note The logger will continue to increment the error counter beyond
+    ///   this value but, once reached, it will not invoke the error callback.
     /// @param maxErrors The number of allowed errors
     void setMaxErrors(const size_t maxErrors = 0);
     /// @brief Returns the number of allowed errors
@@ -194,8 +199,8 @@ class OPENVDB_AX_API Logger
 
     friend class ::TestLogger;
 
-    std::function<void(const std::string&)> mErrorOutput;
-    std::function<void(const std::string&)> mWarningOutput;
+    OutputFunction mErrorOutput;
+    OutputFunction mWarningOutput;
 
     size_t mNumErrors;
     size_t mNumWarnings;