From 32fd2e217a23abbf480d582ee06f814ed7081af5 Mon Sep 17 00:00:00 2001 From: Nick Avramoussis <4256455+Idclip@users.noreply.github.com> Date: Sun, 3 Apr 2022 16:46:59 +0100 Subject: [PATCH 1/2] Various AX improvements: - Fixed a regression in ax::run which wouldn't propagate exceptions - Improved logger exit handling in the compiler - Improved the behaviour of ax::parse for invalid ASTs Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com> --- openvdb_ax/openvdb_ax/ast/Parse.cc | 7 ++- openvdb_ax/openvdb_ax/ast/Parse.h | 24 +++++---- openvdb_ax/openvdb_ax/ax.cc | 32 ++++------- .../openvdb_ax/codegen/ComputeGenerator.cc | 5 +- .../codegen/PointComputeGenerator.cc | 3 +- .../codegen/VolumeComputeGenerator.cc | 3 +- openvdb_ax/openvdb_ax/compiler/Compiler.cc | 16 ++++-- openvdb_ax/openvdb_ax/compiler/Compiler.h | 15 +++--- openvdb_ax/openvdb_ax/compiler/Logger.cc | 16 +++--- openvdb_ax/openvdb_ax/compiler/Logger.h | 19 ++++--- .../openvdb_ax/test/backend/TestLogger.cc | 2 + .../openvdb_ax/test/compiler/TestAXRun.cc | 53 +++++++++++++------ .../test/compiler/TestPointExecutable.cc | 2 +- .../test/compiler/TestVolumeExecutable.cc | 2 +- .../test/frontend/TestSyntaxFailures.cc | 24 +++++++-- openvdb_ax/openvdb_ax/test/util.h | 5 +- 16 files changed, 140 insertions(+), 88 deletions(-) diff --git a/openvdb_ax/openvdb_ax/ast/Parse.cc b/openvdb_ax/openvdb_ax/ast/Parse.cc index 7b0d73e33a..a71fda1325 100644 --- a/openvdb_ax/openvdb_ax/ast/Parse.cc +++ b/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 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; } diff --git a/openvdb_ax/openvdb_ax/ast/Parse.h b/openvdb_ax/openvdb_ax/ast/Parse.h index bc2b793312..c2c39fb527 100644 --- a/openvdb_ax/openvdb_ax/ast/Parse.h +++ b/openvdb_ax/openvdb_ax/ast/Parse.h @@ -23,14 +23,19 @@ 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 @@ -38,14 +43,15 @@ namespace ast { 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 diff --git a/openvdb_ax/openvdb_ax/ax.cc b/openvdb_ax/openvdb_ax/ax.cc index a776d15787..6d12780fad 100644 --- a/openvdb_ax/openvdb_ax/ax.cc +++ b/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()) { // 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(*ast, logger); + compiler.compile(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(*ast, logger); + compiler.compile(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(*ast, logger); + compiler.compile(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(*ast, logger); + compiler.compile(ax); + assert(exe); + //Set the attribute bindings exe->setAttributeBindings(bindings); // Execute on the provided volumes diff --git a/openvdb_ax/openvdb_ax/codegen/ComputeGenerator.cc b/openvdb_ax/openvdb_ax/codegen/ComputeGenerator.cc index a9f0ac1bc9..b8348c2004 100644 --- a/openvdb_ax/openvdb_ax/codegen/ComputeGenerator.cc +++ b/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 diff --git a/openvdb_ax/openvdb_ax/codegen/PointComputeGenerator.cc b/openvdb_ax/openvdb_ax/codegen/PointComputeGenerator.cc index e95a0e9787..f1db9653a3 100644 --- a/openvdb_ax/openvdb_ax/codegen/PointComputeGenerator.cc +++ b/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 diff --git a/openvdb_ax/openvdb_ax/codegen/VolumeComputeGenerator.cc b/openvdb_ax/openvdb_ax/codegen/VolumeComputeGenerator.cc index 94a88aa7f1..f99debb799 100644 --- a/openvdb_ax/openvdb_ax/codegen/VolumeComputeGenerator.cc +++ b/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 diff --git a/openvdb_ax/openvdb_ax/compiler/Compiler.cc b/openvdb_ax/openvdb_ax/compiler/Compiler.cc index 5482b61a07..0ff3825a10 100644 --- a/openvdb_ax/openvdb_ax/compiler/Compiler.cc +++ b/openvdb_ax/openvdb_ax/compiler/Compiler.cc @@ -463,7 +463,7 @@ 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 @@ -471,6 +471,8 @@ void verifyTypedAccesses(const ast::Tree& tree, openvdb::ax::Logger& logger) // 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 nameType; auto attributeOp = @@ -504,6 +506,8 @@ void verifyTypedAccesses(const ast::Tree& tree, openvdb::ax::Logger& logger) }; ast::visitNodeType(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(const ast::Tree& syntaxTree, PointDefaultModifier modifier; modifier.traverse(tree.get()); - verifyTypedAccesses(*tree, logger); - const std::vector functionNames { codegen::PointKernelBufferRange::getDefaultName(), codegen::PointKernelAttributeArray::getDefaultName() @@ -787,8 +795,6 @@ Compiler::compile(const ast::Tree& syntaxTree, { using GenT = codegen::codegen_internal::VolumeComputeGenerator; - verifyTypedAccesses(syntaxTree, logger); - const std::vector functionNames { // codegen::VolumeKernelValue::getDefaultName(), // currently unused directly codegen::VolumeKernelBuffer::getDefaultName(), diff --git a/openvdb_ax/openvdb_ax/compiler/Compiler.h b/openvdb_ax/openvdb_ax/compiler/Compiler.h index 423d3b0cc6..6c4500a205 100644 --- a/openvdb_ax/openvdb_ax/compiler/Compiler.h +++ b/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(*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(*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(syntaxTree, logger, data); if (!errors.empty()) { diff --git a/openvdb_ax/openvdb_ax/compiler/Logger.cc b/openvdb_ax/openvdb_ax/compiler/Logger.cc index 203aa25d61..46a83baf32 100644 --- a/openvdb_ax/openvdb_ax/compiler/Logger.cc +++ b/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,8 +187,11 @@ 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(), @@ -196,10 +199,7 @@ bool Logger::error(const std::string& message, 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,6 +215,7 @@ 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(), @@ -222,7 +223,6 @@ bool Logger::warning(const std::string& message, this->getPrintLines(), this->mSettings->mIndent, this->mCode.get())); - ++mNumWarnings; return true; } } diff --git a/openvdb_ax/openvdb_ax/compiler/Logger.h b/openvdb_ax/openvdb_ax/compiler/Logger.h index 7e00fdcb58..850d8b45e1 100644 --- a/openvdb_ax/openvdb_ax/compiler/Logger.h +++ b/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,14 +78,14 @@ 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 @@ -90,14 +93,14 @@ class OPENVDB_AX_API Logger /// 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 mErrorOutput; - std::function mWarningOutput; + OutputFunction mErrorOutput; + OutputFunction mWarningOutput; size_t mNumErrors; size_t mNumWarnings; diff --git a/openvdb_ax/openvdb_ax/test/backend/TestLogger.cc b/openvdb_ax/openvdb_ax/test/backend/TestLogger.cc index 8032d949d2..c58a670c5f 100644 --- a/openvdb_ax/openvdb_ax/test/backend/TestLogger.cc +++ b/openvdb_ax/openvdb_ax/test/backend/TestLogger.cc @@ -249,5 +249,7 @@ TestLogger::testMaxErrors() CPPUNIT_ASSERT(logger.error(message, location)); CPPUNIT_ASSERT(!logger.error(message, location)); CPPUNIT_ASSERT(!logger.error(message, location)); + // setMaxErrors doesn't limit the error counter + CPPUNIT_ASSERT_EQUAL(size_t(3), logger.errors()); } diff --git a/openvdb_ax/openvdb_ax/test/compiler/TestAXRun.cc b/openvdb_ax/openvdb_ax/test/compiler/TestAXRun.cc index 655f31eadb..90c666e9e6 100644 --- a/openvdb_ax/openvdb_ax/test/compiler/TestAXRun.cc +++ b/openvdb_ax/openvdb_ax/test/compiler/TestAXRun.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: MPL-2.0 #include +#include #include #include @@ -16,10 +17,12 @@ class TestAXRun : public CppUnit::TestCase CPPUNIT_TEST_SUITE(TestAXRun); CPPUNIT_TEST(singleRun); CPPUNIT_TEST(multiRun); + CPPUNIT_TEST(regressions); CPPUNIT_TEST_SUITE_END(); void singleRun(); void multiRun(); + void regressions(); }; CPPUNIT_TEST_SUITE_REGISTRATION(TestAXRun); @@ -31,9 +34,6 @@ TestAXRun::singleRun() f.setName("a"); f.tree().setValueOn({0,0,0}, 0.0f); - // test lexer errors which return an invlid AST - openvdb::ax::run("@c = 1.0f", f); // no-semicolon - openvdb::ax::run("@a = 1.0f;", f); CPPUNIT_ASSERT_EQUAL(1.0f, f.tree().getValue({0,0,0})); @@ -65,17 +65,6 @@ TestAXRun::singleRun() void TestAXRun::multiRun() { - { - // test error on points and volumes - openvdb::FloatGrid::Ptr f(new openvdb::FloatGrid); - openvdb::points::PointDataGrid::Ptr p(new openvdb::points::PointDataGrid); - std::vector v1 { f, p }; - CPPUNIT_ASSERT_THROW(openvdb::ax::run("@a = 1.0f;", v1), openvdb::AXCompilerError); - - std::vector v2 { p, f }; - CPPUNIT_ASSERT_THROW(openvdb::ax::run("@a = 1.0f;", v2), openvdb::AXCompilerError); - } - { // multi volumes openvdb::FloatGrid::Ptr f1(new openvdb::FloatGrid); @@ -86,9 +75,6 @@ TestAXRun::multiRun() f2->tree().setValueOn({0,0,0}, 0.0f); std::vector v { f1, f2 }; - // test lexer errors which return an invlid AST - openvdb::ax::run("@c = 1.0f", v); // no-semicolon - openvdb::ax::run("@a = @b = 1;", v); CPPUNIT_ASSERT_EQUAL(1.0f, f1->tree().getValue({0,0,0})); CPPUNIT_ASSERT_EQUAL(1.0f, f2->tree().getValue({0,0,0})); @@ -155,3 +141,36 @@ TestAXRun::multiRun() } } +void +TestAXRun::regressions() +{ + openvdb::points::PointDataGrid::Ptr p1(new openvdb::points::PointDataGrid); + openvdb::points::PointDataGrid::Ptr p2(new openvdb::points::PointDataGrid); + openvdb::FloatGrid::Ptr f1(new openvdb::FloatGrid); + openvdb::FloatGrid::Ptr f2(new openvdb::FloatGrid); + std::vector g1 { f1, f2 }; + std::vector g2 { p1, p2 }; + + { + // test error on points and volumes + std::vector v1 { f1, p1 }; + std::vector v2 { p1, f1 }; + CPPUNIT_ASSERT_THROW(openvdb::ax::run("@a = 1.0f;", v1), openvdb::AXCompilerError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("@a = 1.0f;", v2), openvdb::AXCompilerError); + } + + // Various tests which have been caught during developement + + CPPUNIT_ASSERT_THROW(openvdb::ax::run("{} =", g1), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("{} =", g2), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("{} =", *f1), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("{} =", *p1), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("@c = 1.0f", g1), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("@c = 1.0f", g2), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("@c = 1.0f", *f1), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("@c = 1.0f", *p1), openvdb::AXSyntaxError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("if (v@v) {}", g1), openvdb::AXCompilerError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("if (v@v) {}", g2), openvdb::AXCompilerError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("if (v@v) {}", *f1), openvdb::AXCompilerError); + CPPUNIT_ASSERT_THROW(openvdb::ax::run("if (v@v) {}", *p1), openvdb::AXCompilerError); +} diff --git a/openvdb_ax/openvdb_ax/test/compiler/TestPointExecutable.cc b/openvdb_ax/openvdb_ax/test/compiler/TestPointExecutable.cc index c41c5c080d..8a89bce7cb 100644 --- a/openvdb_ax/openvdb_ax/test/compiler/TestPointExecutable.cc +++ b/openvdb_ax/openvdb_ax/test/compiler/TestPointExecutable.cc @@ -200,7 +200,7 @@ TestPointExecutable::testCompilerCases() // with string only CPPUNIT_ASSERT(static_cast(compiler->compile("int i;"))); CPPUNIT_ASSERT_THROW(compiler->compile("i;"), openvdb::AXCompilerError); - CPPUNIT_ASSERT_THROW(compiler->compile("i"), openvdb::AXCompilerError); + CPPUNIT_ASSERT_THROW(compiler->compile("i"), openvdb::AXSyntaxError); // with AST only auto ast = openvdb::ax::ast::parse("i;"); CPPUNIT_ASSERT_THROW(compiler->compile(*ast), openvdb::AXCompilerError); diff --git a/openvdb_ax/openvdb_ax/test/compiler/TestVolumeExecutable.cc b/openvdb_ax/openvdb_ax/test/compiler/TestVolumeExecutable.cc index 34ad330f31..ffaa2657da 100644 --- a/openvdb_ax/openvdb_ax/test/compiler/TestVolumeExecutable.cc +++ b/openvdb_ax/openvdb_ax/test/compiler/TestVolumeExecutable.cc @@ -768,7 +768,7 @@ TestVolumeExecutable::testCompilerCases() // with string only CPPUNIT_ASSERT(static_cast(compiler->compile("int i;"))); CPPUNIT_ASSERT_THROW(compiler->compile("i;"), openvdb::AXCompilerError); - CPPUNIT_ASSERT_THROW(compiler->compile("i"), openvdb::AXCompilerError); + CPPUNIT_ASSERT_THROW(compiler->compile("i"), openvdb::AXSyntaxError); // with AST only auto ast = openvdb::ax::ast::parse("i;"); CPPUNIT_ASSERT_THROW(compiler->compile(*ast), openvdb::AXCompilerError); diff --git a/openvdb_ax/openvdb_ax/test/frontend/TestSyntaxFailures.cc b/openvdb_ax/openvdb_ax/test/frontend/TestSyntaxFailures.cc index e7c68e7ed0..6179df57f1 100644 --- a/openvdb_ax/openvdb_ax/test/frontend/TestSyntaxFailures.cc +++ b/openvdb_ax/openvdb_ax/test/frontend/TestSyntaxFailures.cc @@ -261,6 +261,7 @@ static const std::vector tests { "if + a;", "a + if(true) {};", "{} + {};", + "{} int", "~ + !;", "+ + -;", "; + ;", @@ -567,7 +568,24 @@ static const std::vector tests { "|;", ",;", "!;", - "\\;" + "\\;", + + // Test right associativity. These symbols are defined with %right in the + // parser which forces partial creation of ASTs. ::parse should ensure + // that these cases still produce invalid AST ptrs. + "{} ?", + "{} :", + "{} =", + "{} +=", + "{} -=", + "{} /=", + "{} *=", + "{} %=", + "{} |=", + "{} &=", + "{} ^=", + "{} <<=", + "{} >>=" }; } @@ -587,7 +605,7 @@ CPPUNIT_TEST_SUITE_REGISTRATION(TestSyntaxFailures); void TestSyntaxFailures::testSyntax() { - // Quickly check the above doesn't have multiple occurance + // Quickly check the above doesn't have multiple occurrence // store multiple in a hash map const auto hash = [](const StrWrapper* s) { return std::hash()(s->first); @@ -611,5 +629,3 @@ void TestSyntaxFailures::testSyntax() TEST_SYNTAX_FAILS(tests); } - - diff --git a/openvdb_ax/openvdb_ax/test/util.h b/openvdb_ax/openvdb_ax/test/util.h index 06a6f1924d..0494944460 100644 --- a/openvdb_ax/openvdb_ax/test/util.h +++ b/openvdb_ax/openvdb_ax/test/util.h @@ -11,6 +11,7 @@ #define OPENVDB_AX_UNITTEST_UTIL_HAS_BEEN_INCLUDED #include +#include #include #include #include @@ -34,7 +35,7 @@ const std::string& code = test.first; \ openvdb::ax::ast::Tree::ConstPtr tree = openvdb::ax::ast::parse(code.c_str(), logger);\ std::stringstream str; \ - CPPUNIT_ASSERT_MESSAGE(ERROR_MSG("Unexpected parsing error(s)\n", str.str()), tree); \ + CPPUNIT_ASSERT_MESSAGE(ERROR_MSG("Unexpected parsing error(s)\n", str.str()), tree && !logger.hasError()); \ } \ } \ @@ -45,7 +46,7 @@ logger.clear();\ const std::string& code = test.first; \ openvdb::ax::ast::Tree::ConstPtr tree = openvdb::ax::ast::parse(code.c_str(), logger);\ - CPPUNIT_ASSERT_MESSAGE(ERROR_MSG("Expected parsing error", code), logger.hasError()); \ + CPPUNIT_ASSERT_MESSAGE(ERROR_MSG("Expected parsing error", code), !tree && logger.hasError()); \ } \ } \ From 3ef01fd74a71448becf8e1dabc474bc90b7a7b1a Mon Sep 17 00:00:00 2001 From: Nick Avramoussis <4256455+Idclip@users.noreply.github.com> Date: Thu, 28 Apr 2022 17:10:59 +0100 Subject: [PATCH 2/2] Updating pendingchanges Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com> --- pendingchanges/ax_fixes.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 pendingchanges/ax_fixes.txt diff --git a/pendingchanges/ax_fixes.txt b/pendingchanges/ax_fixes.txt new file mode 100644 index 0000000000..308365fb8b --- /dev/null +++ b/pendingchanges/ax_fixes.txt @@ -0,0 +1,4 @@ +Bug Fixes: + - Fixed a regression in ax::run which wouldn't propagate exceptions + - Fixed a bug where ax::ast::parse could return a partially constructed but invalid AST on failure + - Fixed AX logger exit handling in ax::Compiler::compile and ax::ast::parse