From ce500df92f9b025f96114814eed893734a165e5a Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Thu, 28 Apr 2022 15:50:08 +0200 Subject: [PATCH 1/7] test to pass --- .../expressions/logical_expression_test.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/parser_library/test/expressions/logical_expression_test.cpp b/parser_library/test/expressions/logical_expression_test.cpp index a84a322c3..2d874cbc3 100644 --- a/parser_library/test/expressions/logical_expression_test.cpp +++ b/parser_library/test/expressions/logical_expression_test.cpp @@ -273,3 +273,25 @@ TEST(logical_expressions, parenthesis_around_expressions) EXPECT_EQ(diags.size(), (size_t)2); EXPECT_TRUE(std::all_of(diags.begin(), diags.end(), [](const auto& d) { return d.code == "CE016"; })); } + +TEST(logical_expressions, operator_precedence) +{ + std::string input = + R"( + MACRO + TEST &A,&B + AIF (&A EQ 0 OR &A EQ 1 AND &B EQ 1).END + MNOTE 8,'ERROR' +.END ANOP + MEND + + TEST 0,0 + TEST 0,1 + TEST 1,1 +)"; + analyzer a(input); + a.analyze(); + a.collect_diags(); + + EXPECT_TRUE(a.diags().empty()); +} From e70eb7562b3a3931726cba7a3056d23a502e0349 Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Fri, 29 Apr 2022 15:42:34 +0200 Subject: [PATCH 2/7] passing tests --- .../conditional_assembly/ca_expr_policy.cpp | 62 +++-- .../conditional_assembly/ca_expr_policy.h | 35 ++- .../ca_operator_binary.cpp | 6 - .../terms/ca_expr_list.cpp | 247 ++++++++++-------- .../conditional_assembly/terms/ca_expr_list.h | 9 - .../test/expressions/ca_operator_test.cpp | 3 - .../expressions/logical_expression_test.cpp | 26 +- 7 files changed, 228 insertions(+), 160 deletions(-) diff --git a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp index 03abde8b3..607e8147d 100644 --- a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp +++ b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.cpp @@ -26,13 +26,6 @@ bool ca_character_policy::is_unary(ca_expr_ops op) || op == ca_expr_ops::UPPER; } -bool ca_arithmetic_policy::multiple_words(ca_expr_ops) { return false; } -bool ca_binary_policy::multiple_words(ca_expr_ops op) -{ - return op == ca_expr_ops::AND || op == ca_expr_ops::OR || op == ca_expr_ops::XOR; -} -bool ca_character_policy::multiple_words(ca_expr_ops) { return false; } - bool ca_arithmetic_policy::is_binary(ca_expr_ops op) { return op == ca_expr_ops::SLA || op == ca_expr_ops::SLL || op == ca_expr_ops::SRA || op == ca_expr_ops::SRL @@ -44,8 +37,7 @@ bool ca_binary_policy::is_binary(ca_expr_ops op) { return op == ca_expr_ops::EQ || op == ca_expr_ops::NE || op == ca_expr_ops::LE || op == ca_expr_ops::LT || op == ca_expr_ops::GE || op == ca_expr_ops::GT || op == ca_expr_ops::AND || op == ca_expr_ops::OR - || op == ca_expr_ops::XOR || op == ca_expr_ops::AND_NOT || op == ca_expr_ops::OR_NOT - || op == ca_expr_ops::XOR_NOT; + || op == ca_expr_ops::XOR; } bool ca_character_policy::is_binary(ca_expr_ops) { return false; } @@ -156,13 +148,10 @@ int ca_binary_policy::get_priority(ca_expr_ops op) case ca_expr_ops::NOT: return 1; case ca_expr_ops::AND: - case ca_expr_ops::AND_NOT: return 2; case ca_expr_ops::OR: - case ca_expr_ops::OR_NOT: return 3; case ca_expr_ops::XOR: - case ca_expr_ops::XOR_NOT: return 4; default: return 0; @@ -289,9 +278,6 @@ ca_expr_ops get_expr_operator(const std::string& op) S2O(SRL); S2O(FIND); S2O(INDEX); - S2O(AND_NOT); - S2O(OR_NOT); - S2O(XOR_NOT); S2O(EQ); S2O(NE); S2O(LE); @@ -312,8 +298,54 @@ ca_expr_ops get_expr_operator(const std::string& op) } ca_expr_ops ca_arithmetic_policy::get_operator(const std::string& symbol) { return get_expr_operator(symbol); } +std::variant ca_arithmetic_policy::get_operator_properties( + const std::string& symbol) +{ + auto o = get_operator(symbol); + if (o == ca_expr_ops::UNKNOWN) + return std::monostate(); + bool unary = is_unary(o); + bool binary = is_binary(o); + if (!unary && !binary) + return invalid_by_policy(); + return ca_expr_op { o, get_priority(o), binary, false, unary }; +} + ca_expr_ops ca_binary_policy::get_operator(const std::string& symbol) { return get_expr_operator(symbol); } +std::variant ca_binary_policy::get_operator_properties( + const std::string& symbol) +{ + auto o = get_operator(symbol); + if (o == ca_expr_ops::UNKNOWN) + return std::monostate(); + bool unary = is_unary(o); + bool binary = is_binary(o); + if (!unary && !binary) + return invalid_by_policy(); + return ca_expr_op { + o, + get_priority(o), + binary, + o == ca_expr_ops::EQ || o == ca_expr_ops::NE || o == ca_expr_ops::LE || o == ca_expr_ops::LT + || o == ca_expr_ops::GE || o == ca_expr_ops::GT, + unary, + }; +} + ca_expr_ops ca_character_policy::get_operator(const std::string& symbol) { return get_expr_operator(symbol); } +std::variant ca_character_policy::get_operator_properties( + const std::string& symbol) +{ + auto o = get_operator(symbol); + if (o == ca_expr_ops::UNKNOWN) + return std::monostate(); + bool unary = is_unary(o); + bool binary = is_binary(o); + if (!unary && !binary) + return invalid_by_policy(); + return ca_expr_op { o, get_priority(o), binary, false, unary }; +} + ca_expr_funcs ca_arithmetic_policy::get_function(const std::string& symbol) { diff --git a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h index 7484ccb2c..c7693f013 100644 --- a/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h +++ b/parser_library/src/expressions/conditional_assembly/ca_expr_policy.h @@ -15,6 +15,8 @@ #ifndef HLASMPLUGIN_PARSERLIBRARY_CA_EXPR_POLICY_H #define HLASMPLUGIN_PARSERLIBRARY_CA_EXPR_POLICY_H +#include + #include "context/common_types.h" #include "context/id_storage.h" @@ -34,9 +36,6 @@ enum class ca_expr_ops INDEX, // logical - AND_NOT, - OR_NOT, - XOR_NOT, EQ, NE, LE, @@ -60,6 +59,18 @@ enum class ca_expr_ops UNKNOWN }; +struct ca_expr_op +{ + ca_expr_ops op; + int priority; + bool binary; + bool requires_terms; + bool right_assoc; +}; + +struct invalid_by_policy +{}; + enum class ca_expr_funcs { // arithmetic @@ -120,9 +131,6 @@ class ca_arithmetic_policy // is binary arithmetic operation static bool is_binary(ca_expr_ops op); - // true if op can consist of multiple words (eg AND NOT) - static bool multiple_words(ca_expr_ops op); - // get priority relative to rest of arithmetic operators static int get_priority(ca_expr_ops op); @@ -135,6 +143,9 @@ class ca_arithmetic_policy // transforms string operator to enum static ca_expr_ops get_operator(const std::string& symbol); + static std::variant get_operator_properties( + const std::string& symbol); + // transforms string function to enum static ca_expr_funcs get_function(const std::string& symbol); @@ -157,9 +168,6 @@ class ca_binary_policy // is binary binary operation static bool is_binary(ca_expr_ops op); - // true if op can consist of multiple words (eg AND NOT) - static bool multiple_words(ca_expr_ops op); - // get priority relative to rest of binary operators static int get_priority(ca_expr_ops op); @@ -172,6 +180,9 @@ class ca_binary_policy // transforms string operator to enum static ca_expr_ops get_operator(const std::string& symbol); + static std::variant get_operator_properties( + const std::string& symbol); + // transforms string function to enum static ca_expr_funcs get_function(const std::string& symbol); @@ -194,9 +205,6 @@ class ca_character_policy // is binary character operation static bool is_binary(ca_expr_ops op); - // true if op can consist of multiple words (eg AND NOT) - static bool multiple_words(ca_expr_ops op); - // get priority relative to rest of character operators static int get_priority(ca_expr_ops op); @@ -209,6 +217,9 @@ class ca_character_policy // transforms string operator to enum static ca_expr_ops get_operator(const std::string& symbol); + static std::variant get_operator_properties( + const std::string& symbol); + // transforms string function to enum static ca_expr_funcs get_function(const std::string& symbol); diff --git a/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp b/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp index 63591cb79..d176e9ce3 100644 --- a/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp +++ b/parser_library/src/expressions/conditional_assembly/ca_operator_binary.cpp @@ -205,12 +205,6 @@ context::SET_t ca_function_binary_operator::operation( return lhs.access_b() || rhs.access_b(); case ca_expr_ops::XOR: return lhs.access_b() != rhs.access_b(); - case ca_expr_ops::AND_NOT: - return lhs.access_b() && !rhs.access_b(); - case ca_expr_ops::OR_NOT: - return lhs.access_b() || !rhs.access_b(); - case ca_expr_ops::XOR_NOT: - return lhs.access_b() != !rhs.access_b(); default: break; } diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp index d7a4e619c..6938e8bf7 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp @@ -15,6 +15,7 @@ #include "ca_expr_list.h" #include +#include #include "../ca_operator_binary.h" #include "../ca_operator_unary.h" @@ -113,132 +114,168 @@ void ca_expr_list::unknown_functions_to_operators() template void ca_expr_list::resolve(diagnostic_op_consumer& diags) { - if (expr_list.empty()) + struct term_entry { - diags.add_diagnostic(diagnostic_op::error_CE003(expr_range)); - return; - } - - size_t it = 0; - bool err = false; - - ca_expr_ptr final_expr = retrieve_term::policy_t>(it, 0, diags); - err |= final_expr == nullptr; - - while (it != expr_list.size() && !err) + ca_expr_ptr term; + size_t i; + }; + struct op_entry { - auto op_range = expr_list[it]->expr_range; - - auto [prio, op_type] = retrieve_binary_operator::policy_t>(it, err, diags); - - auto r_expr = retrieve_term::policy_t>(++it, prio, diags); - err |= r_expr == nullptr; + size_t i; + ca_expr_ops op_type; + int priority; + bool binary; + bool right_assoc; + range r; + }; + using expr_policy = typename ca_expr_traits::policy_t; + + std::stack terms; + std::stack op_stack; + + const auto reduce_stack = [&terms, &op_stack, &diags]() { + auto op = std::move(op_stack.top()); + op_stack.pop(); + if (terms.size() < 1 + op.binary) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(terms.size() < op.binary ? op.r.start : op.r.end))); + return false; + } + if (op.binary) + { + auto right = std::move(terms.top()); + terms.pop(); + auto left = std::move(terms.top()); + terms.pop(); - final_expr = std::make_unique( - std::move(final_expr), std::move(r_expr), op_type, context::object_traits::type_enum, op_range); - } + if (left.i > op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.start))); + return false; + } + if (right.i < op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + return false; + } - if (err) - { - expr_list.clear(); - return; - } + terms.push({ + std::make_unique(std::move(left.term), + std::move(right.term), + op.op_type, + context::object_traits::type_enum, + op.r), + left.i, + }); + } + else + { + auto right = std::move(terms.top()); + terms.pop(); - // resolve created tree - final_expr->resolve_expression_tree(context::object_traits::type_enum, diags); + if (right.i < op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + return false; + } - // move resolved tree to the front of the array - expr_list.clear(); - expr_list.emplace_back(std::move(final_expr)); -} + terms.push({ + std::make_unique( + std::move(right.term), op.op_type, expr_policy::set_type, op.r), + op.i, + }); + } + return true; + }; -template -ca_expr_ptr ca_expr_list::retrieve_term(size_t& it, int priority, diagnostic_op_consumer& diags) -{ - // list is exhausted - if (it == expr_list.size()) + auto i = (size_t)-1; + bool prefer_next_term = true; + bool last_was_terminal = false; + for (auto& curr_expr : expr_list) { - auto r = expr_list[it - 1]->expr_range; - r.start = r.end; - r.end.column++; - diags.add_diagnostic(diagnostic_op::error_CE003(r)); - return nullptr; + ++i; + // is unary op + if (is_symbol(curr_expr)) + { + const auto& symbol = get_symbol(curr_expr); + auto op_type_var = expr_policy::get_operator_properties(symbol); + if (std::holds_alternative(op_type_var)) + { + // fallback to term + } + else if (!prefer_next_term && std::holds_alternative(op_type_var)) + { + diags.add_diagnostic(diagnostic_op::error_CE002(symbol, curr_expr->expr_range)); + expr_list.clear(); + return; + } + else if (const auto& op_type = std::get(op_type_var); !(prefer_next_term && op_type.binary)) + { + while (!op_stack.empty()) + { + if (op_stack.top().priority > op_type.priority) + break; + if (op_stack.top().priority == op_type.priority && op_type.right_assoc) + break; + if (!reduce_stack()) + { + expr_list.clear(); + return; + } + } + op_stack.push({ + i, + op_type.op, + op_type.priority, + op_type.binary, + op_type.right_assoc, + curr_expr->expr_range, + }); + if (op_type.requires_terms && !last_was_terminal) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(curr_expr->expr_range.start))); + expr_list.clear(); + return; + } + prefer_next_term = op_type.binary || op_type.requires_terms; + last_was_terminal = false; + continue; + } + } + terms.push({ std::move(curr_expr), i }); + prefer_next_term = false; + last_was_terminal = true; } - // first possible term - auto& curr_expr = expr_list[it]; - - // is unary op - if (is_symbol(curr_expr)) + while (!op_stack.empty()) { - if (auto op_type = EXPR_POLICY::get_operator(get_symbol(curr_expr)); EXPR_POLICY::is_unary(op_type)) + if (!reduce_stack()) { - auto new_expr = retrieve_term(++it, EXPR_POLICY::get_priority(op_type), diags); - if (!new_expr) - return nullptr; - - return std::make_unique( - std::move(new_expr), op_type, EXPR_POLICY::set_type, curr_expr->expr_range); + expr_list.clear(); + return; } } - - // is only term - if (it + 1 == expr_list.size()) - return std::move(expr_list[it++]); - - // tries to get binary operator - auto op_it = ++it; - auto op_range = expr_list[op_it]->expr_range; - bool err = false; - - auto [op_prio, op_type] = retrieve_binary_operator(op_it, err, diags); - if (err) - return nullptr; - - // if operator is of lower priority than the calling operator, finish - if (op_prio >= priority) - return std::move(curr_expr); - else - it = op_it; - - auto right_expr = retrieve_term(++it, op_prio, diags); - if (!right_expr) - return nullptr; - - return std::make_unique( - std::move(curr_expr), std::move(right_expr), op_type, EXPR_POLICY::set_type, op_range); -} - -template -std::pair ca_expr_list::retrieve_binary_operator(size_t& it, bool& err, diagnostic_op_consumer& diags) -{ - const auto& op = expr_list[it]; - - if (!is_symbol(op)) + if (terms.empty()) { - diags.add_diagnostic(diagnostic_op::error_CE001(expr_range)); - err = true; - return std::make_pair(0, ca_expr_ops::UNKNOWN); + diags.add_diagnostic(diagnostic_op::error_CE003(expr_range)); + expr_list.clear(); + return; } - else if (!EXPR_POLICY::is_operator(EXPR_POLICY::get_operator(get_symbol(op)))) + if (terms.size() > 1) { - diags.add_diagnostic(diagnostic_op::error_CE002(get_symbol(op), expr_range)); - err = true; - return std::make_pair(0, ca_expr_ops::UNKNOWN); + diags.add_diagnostic(diagnostic_op::error_CE001(range(terms.top().term->expr_range))); + expr_list.clear(); + return; } - ca_expr_ops op_type = EXPR_POLICY::get_operator(get_symbol(expr_list[it])); + ca_expr_ptr final_expr = std::move(terms.top().term); - if (EXPR_POLICY::multiple_words(op_type) && it + 1 < expr_list.size() && is_symbol(expr_list[it + 1]) - && get_symbol(expr_list[it + 1]) == "NOT") - { - op_type = EXPR_POLICY::get_operator(get_symbol(expr_list[it]) + "_NOT"); - ++it; - } - - auto op_prio = EXPR_POLICY::get_priority(op_type); + // resolve created tree + final_expr->resolve_expression_tree(context::object_traits::type_enum, diags); - return std::make_pair(op_prio, op_type); + // move resolved tree to the front of the array + expr_list.clear(); + expr_list.emplace_back(std::move(final_expr)); } } // namespace hlasm_plugin::parser_library::expressions diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h index 72458bf7b..fa4591f66 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.h @@ -57,15 +57,6 @@ class ca_expr_list final : public ca_expression // each loop iteration it pastes them together and continue until list is exhausted template void resolve(diagnostic_op_consumer& diags); - // retrieves single term with possible unary operators before it - // also checks for following binary operator, - // if it has higher prio than the current one, recursively calls retrieve_term for the second term for the higher - // priority operator - template - ca_expr_ptr retrieve_term(size_t& it, int priority, diagnostic_op_consumer& diags); - // retrieves following binary operator with its priority - template - std::pair retrieve_binary_operator(size_t& it, bool& err, diagnostic_op_consumer& diags); }; } // namespace hlasm_plugin::parser_library::expressions diff --git a/parser_library/test/expressions/ca_operator_test.cpp b/parser_library/test/expressions/ca_operator_test.cpp index e0cf17f47..c5818b196 100644 --- a/parser_library/test/expressions/ca_operator_test.cpp +++ b/parser_library/test/expressions/ca_operator_test.cpp @@ -117,11 +117,8 @@ INSTANTIATE_TEST_SUITE_P(op_parameters_suite, op_test_param { ca_expr_ops::NOT, SET_t_enum::A_TYPE, { 10 }, -11, "NOTA" }, op_test_param { ca_expr_ops::AND, SET_t_enum::B_TYPE, { false, true }, false, "ANDB" }, - op_test_param { ca_expr_ops::AND_NOT, SET_t_enum::B_TYPE, { true, false }, true, "AND_NOT" }, op_test_param { ca_expr_ops::OR, SET_t_enum::B_TYPE, { false, true }, true, "ORB" }, - op_test_param { ca_expr_ops::OR_NOT, SET_t_enum::B_TYPE, { false, true }, false, "OR_NOT" }, op_test_param { ca_expr_ops::XOR, SET_t_enum::B_TYPE, { false, true }, true, "XORB" }, - op_test_param { ca_expr_ops::XOR_NOT, SET_t_enum::B_TYPE, { true, false }, false, "XOR_NOT" }, op_test_param { ca_expr_ops::NOT, SET_t_enum::B_TYPE, { false }, true, "NOTB" }, op_test_param { ca_expr_ops::EQ, SET_t_enum::B_TYPE, { 1, 0 }, false, "EQA" }, diff --git a/parser_library/test/expressions/logical_expression_test.cpp b/parser_library/test/expressions/logical_expression_test.cpp index 2d874cbc3..cbd51b70f 100644 --- a/parser_library/test/expressions/logical_expression_test.cpp +++ b/parser_library/test/expressions/logical_expression_test.cpp @@ -276,8 +276,15 @@ TEST(logical_expressions, parenthesis_around_expressions) TEST(logical_expressions, operator_precedence) { - std::string input = - R"( + for (const auto& [args, expected] : std::initializer_list> { + { "0,0", true }, + { "0,1", true }, + { "1,0", false }, + { "1,1", true }, + }) + { + std::string input = + R"( MACRO TEST &A,&B AIF (&A EQ 0 OR &A EQ 1 AND &B EQ 1).END @@ -285,13 +292,12 @@ TEST(logical_expressions, operator_precedence) .END ANOP MEND - TEST 0,0 - TEST 0,1 - TEST 1,1 -)"; - analyzer a(input); - a.analyze(); - a.collect_diags(); + TEST )" + + args; + analyzer a(input); + a.analyze(); + a.collect_diags(); - EXPECT_TRUE(a.diags().empty()); + EXPECT_EQ(a.diags().empty(), expected) << args; + } } From 300b41dea5ad901942c0cdfea7dca932d7f7dd7a Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Fri, 29 Apr 2022 16:24:12 +0200 Subject: [PATCH 3/7] refactor --- .../terms/ca_expr_list.cpp | 185 ++++++++++-------- 1 file changed, 108 insertions(+), 77 deletions(-) diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp index 6938e8bf7..f3fae3ae6 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp @@ -111,29 +111,80 @@ void ca_expr_list::unknown_functions_to_operators() } } +namespace { + +struct term_entry +{ + ca_expr_ptr term; + size_t i; +}; +struct op_entry +{ + size_t i; + ca_expr_ops op_type; + int priority; + bool binary; + bool right_assoc; + range r; +}; + template -void ca_expr_list::resolve(diagnostic_op_consumer& diags) +struct resolve_stacks { - struct term_entry - { - ca_expr_ptr term; - size_t i; - }; - struct op_entry - { - size_t i; - ca_expr_ops op_type; - int priority; - bool binary; - bool right_assoc; - range r; - }; using expr_policy = typename ca_expr_traits::policy_t; std::stack terms; std::stack op_stack; - const auto reduce_stack = [&terms, &op_stack, &diags]() { + void push_term(term_entry t) { terms.push(std::move(t)); } + void push_op(op_entry op) { op_stack.push(std::move(op)); } + + bool reduce_binary(const op_entry& op, diagnostic_op_consumer& diags) + { + auto right = std::move(terms.top()); + terms.pop(); + auto left = std::move(terms.top()); + terms.pop(); + + if (left.i > op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.start))); + return false; + } + if (right.i < op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + return false; + } + + terms.push({ + std::make_unique( + std::move(left.term), std::move(right.term), op.op_type, context::object_traits::type_enum, op.r), + left.i, + }); + return true; + } + bool reduce_unary(const op_entry& op, diagnostic_op_consumer& diags) + { + auto right = std::move(terms.top()); + terms.pop(); + + if (right.i < op.i) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + return false; + } + + terms.push({ + std::make_unique( + std::move(right.term), op.op_type, expr_policy::set_type, op.r), + op.i, + }); + return true; + } + + bool reduce_stack_entry(diagnostic_op_consumer& diags) + { auto op = std::move(op_stack.top()); op_stack.pop(); if (terms.size() < 1 + op.binary) @@ -141,52 +192,42 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) diags.add_diagnostic(diagnostic_op::error_CE003(range(terms.size() < op.binary ? op.r.start : op.r.end))); return false; } - if (op.binary) - { - auto right = std::move(terms.top()); - terms.pop(); - auto left = std::move(terms.top()); - terms.pop(); + return op.binary ? reduce_binary(op, diags) : reduce_unary(op, diags); + } - if (left.i > op.i) - { - diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.start))); - return false; - } - if (right.i < op.i) - { - diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + bool reduce_stack(diagnostic_op_consumer& diags, int prio_limit, bool right_assoc) + { + while (!op_stack.empty()) + { + if (op_stack.top().priority > prio_limit) + break; + if (op_stack.top().priority == prio_limit && right_assoc) + break; + if (!reduce_stack_entry(diags)) return false; - } - - terms.push({ - std::make_unique(std::move(left.term), - std::move(right.term), - op.op_type, - context::object_traits::type_enum, - op.r), - left.i, - }); } - else - { - auto right = std::move(terms.top()); - terms.pop(); + return true; + } - if (right.i < op.i) - { - diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); + bool reduce_stack_all(diagnostic_op_consumer& diags) + { + while (!op_stack.empty()) + { + if (!reduce_stack_entry(diags)) return false; - } - - terms.push({ - std::make_unique( - std::move(right.term), op.op_type, expr_policy::set_type, op.r), - op.i, - }); } return true; - }; + } +}; + +} // namespace + +template +void ca_expr_list::resolve(diagnostic_op_consumer& diags) +{ + using expr_policy = typename ca_expr_traits::policy_t; + + resolve_stacks stacks; auto i = (size_t)-1; bool prefer_next_term = true; @@ -211,19 +252,12 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) } else if (const auto& op_type = std::get(op_type_var); !(prefer_next_term && op_type.binary)) { - while (!op_stack.empty()) + if (!stacks.reduce_stack(diags, op_type.priority, op_type.right_assoc)) { - if (op_stack.top().priority > op_type.priority) - break; - if (op_stack.top().priority == op_type.priority && op_type.right_assoc) - break; - if (!reduce_stack()) - { - expr_list.clear(); - return; - } + expr_list.clear(); + return; } - op_stack.push({ + stacks.push_op({ i, op_type.op, op_type.priority, @@ -242,33 +276,30 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) continue; } } - terms.push({ std::move(curr_expr), i }); + stacks.push_term({ std::move(curr_expr), i }); prefer_next_term = false; last_was_terminal = true; } - while (!op_stack.empty()) + if (!stacks.reduce_stack_all(diags)) { - if (!reduce_stack()) - { - expr_list.clear(); - return; - } + expr_list.clear(); + return; } - if (terms.empty()) + if (stacks.terms.empty()) { diags.add_diagnostic(diagnostic_op::error_CE003(expr_range)); expr_list.clear(); return; } - if (terms.size() > 1) + if (stacks.terms.size() > 1) { - diags.add_diagnostic(diagnostic_op::error_CE001(range(terms.top().term->expr_range))); + diags.add_diagnostic(diagnostic_op::error_CE001(range(stacks.terms.top().term->expr_range))); expr_list.clear(); return; } - ca_expr_ptr final_expr = std::move(terms.top().term); + ca_expr_ptr final_expr = std::move(stacks.terms.top().term); // resolve created tree final_expr->resolve_expression_tree(context::object_traits::type_enum, diags); From 80932dbca04ed3ddd2a92f7387330f05e53d4f30 Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Fri, 29 Apr 2022 18:14:00 +0200 Subject: [PATCH 4/7] more validation --- .../conditional_assembly/terms/ca_expr_list.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp index f3fae3ae6..34d788320 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp @@ -117,6 +117,7 @@ struct term_entry { ca_expr_ptr term; size_t i; + bool simple_term; }; struct op_entry { @@ -125,6 +126,7 @@ struct op_entry int priority; bool binary; bool right_assoc; + bool requires_terms; range r; }; @@ -146,12 +148,12 @@ struct resolve_stacks auto left = std::move(terms.top()); terms.pop(); - if (left.i > op.i) + if (op.requires_terms && !left.simple_term || left.i > op.i) { diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.start))); return false; } - if (right.i < op.i) + if (op.requires_terms && !right.simple_term || right.i < op.i) { diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); return false; @@ -161,6 +163,7 @@ struct resolve_stacks std::make_unique( std::move(left.term), std::move(right.term), op.op_type, context::object_traits::type_enum, op.r), left.i, + false, }); return true; } @@ -179,6 +182,7 @@ struct resolve_stacks std::make_unique( std::move(right.term), op.op_type, expr_policy::set_type, op.r), op.i, + false, }); return true; } @@ -263,6 +267,7 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) op_type.priority, op_type.binary, op_type.right_assoc, + op_type.requires_terms, curr_expr->expr_range, }); if (op_type.requires_terms && !last_was_terminal) @@ -276,7 +281,7 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) continue; } } - stacks.push_term({ std::move(curr_expr), i }); + stacks.push_term({ std::move(curr_expr), i, true }); prefer_next_term = false; last_was_terminal = true; } From dd5cbbdc1a607c0fe3799cc475f21d6577c1b64b Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Fri, 29 Apr 2022 18:14:05 +0200 Subject: [PATCH 5/7] less warnings --- language_server/test/virtual_file_provider_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/language_server/test/virtual_file_provider_test.cpp b/language_server/test/virtual_file_provider_test.cpp index b8b26e0bb..eb382381b 100644 --- a/language_server/test/virtual_file_provider_test.cpp +++ b/language_server/test/virtual_file_provider_test.cpp @@ -46,7 +46,7 @@ TEST(virtual_file_provider, predicate) TEST(virtual_file_provider, file_missing) { - ws_mngr_mock ws_mngr; + NiceMock ws_mngr; json_sink_mock sink; virtual_file_provider vfp(ws_mngr, sink); @@ -75,7 +75,7 @@ TEST(virtual_file_provider, file_missing) TEST(virtual_file_provider, file_present) { - ws_mngr_mock ws_mngr; + NiceMock ws_mngr; json_sink_mock sink; virtual_file_provider vfp(ws_mngr, sink); From 1bb7d71c0f0adcf4eaf6c07674f69d1c130c5fa5 Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Fri, 29 Apr 2022 18:15:52 +0200 Subject: [PATCH 6/7] changelog --- clients/vscode-hlasmplugin/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/clients/vscode-hlasmplugin/CHANGELOG.md b/clients/vscode-hlasmplugin/CHANGELOG.md index ff51573ce..525182855 100644 --- a/clients/vscode-hlasmplugin/CHANGELOG.md +++ b/clients/vscode-hlasmplugin/CHANGELOG.md @@ -20,6 +20,7 @@ - Empty arrays now behave similarly to other subscripted variables in macro tracer - Forward structured macro parameters correctly - Empty operands ignored by the SETx instructions +- Incorrect operator precedence in conditional assembly expressions ## [1.1.0](https://github.com/eclipse/che-che4z-lsp-for-hlasm/compare/1.0.0...1.1.0) (2022-03-29) From d20897a11454e00e0b4dd0639f6dc8a1111aa972 Mon Sep 17 00:00:00 2001 From: Slavomir Kucera Date: Mon, 2 May 2022 13:51:04 +0200 Subject: [PATCH 7/7] fix bug + comment --- .../terms/ca_expr_list.cpp | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp index 34d788320..4e7cd1fd8 100644 --- a/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp +++ b/parser_library/src/expressions/conditional_assembly/terms/ca_expr_list.cpp @@ -167,12 +167,13 @@ struct resolve_stacks }); return true; } + bool reduce_unary(const op_entry& op, diagnostic_op_consumer& diags) { auto right = std::move(terms.top()); terms.pop(); - if (right.i < op.i) + if (op.requires_terms && !right.simple_term || right.i < op.i) { diags.add_diagnostic(diagnostic_op::error_CE003(range(op.r.end))); return false; @@ -239,7 +240,6 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) for (auto& curr_expr : expr_list) { ++i; - // is unary op if (is_symbol(curr_expr)) { const auto& symbol = get_symbol(curr_expr); @@ -248,14 +248,26 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) { // fallback to term } - else if (!prefer_next_term && std::holds_alternative(op_type_var)) + else if (std::holds_alternative(op_type_var)) { - diags.add_diagnostic(diagnostic_op::error_CE002(symbol, curr_expr->expr_range)); - expr_list.clear(); - return; + if (!prefer_next_term) + { + diags.add_diagnostic(diagnostic_op::error_CE002(symbol, curr_expr->expr_range)); + expr_list.clear(); + return; + } + // fallback to term } - else if (const auto& op_type = std::get(op_type_var); !(prefer_next_term && op_type.binary)) + else if (const auto& op_type = std::get(op_type_var); + !(prefer_next_term && op_type.binary)) // ... AND AND is interpreted as AND term, + // ... AND NOT ... is apparently not { + if (op_type.requires_terms && !last_was_terminal) + { + diags.add_diagnostic(diagnostic_op::error_CE003(range(curr_expr->expr_range.start))); + expr_list.clear(); + return; + } if (!stacks.reduce_stack(diags, op_type.priority, op_type.right_assoc)) { expr_list.clear(); @@ -270,12 +282,6 @@ void ca_expr_list::resolve(diagnostic_op_consumer& diags) op_type.requires_terms, curr_expr->expr_range, }); - if (op_type.requires_terms && !last_was_terminal) - { - diags.add_diagnostic(diagnostic_op::error_CE003(range(curr_expr->expr_range.start))); - expr_list.clear(); - return; - } prefer_next_term = op_type.binary || op_type.requires_terms; last_was_terminal = false; continue;