From 20e56db4894fd64958712f6b755af006b6bb8622 Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Sun, 7 May 2023 20:20:04 +0200 Subject: [PATCH 1/7] Apply clang-format to expression{.cc,.h,_test.cc} --- verilog/CST/expression.cc | 76 ++++++++++++------------ verilog/CST/expression.h | 38 ++++++------ verilog/CST/expression_test.cc | 102 ++++++++++++++++----------------- 3 files changed, 108 insertions(+), 108 deletions(-) diff --git a/verilog/CST/expression.cc b/verilog/CST/expression.cc index dec6e020aa..88eb35dac0 100644 --- a/verilog/CST/expression.cc +++ b/verilog/CST/expression.cc @@ -41,78 +41,78 @@ using verible::SyntaxTreeLeaf; using verible::SyntaxTreeNode; using verible::TreeSearchMatch; -bool IsExpression(const verible::SymbolPtr& symbol_ptr) { +bool IsExpression(const verible::SymbolPtr &symbol_ptr) { if (symbol_ptr == nullptr) return false; if (symbol_ptr->Kind() != SymbolKind::kNode) return false; - const auto& node = down_cast(*symbol_ptr); + const auto &node = down_cast(*symbol_ptr); return node.MatchesTag(NodeEnum::kExpression); } -bool IsZero(const Symbol& expr) { - const Symbol* child = verible::DescendThroughSingletons(expr); +bool IsZero(const Symbol &expr) { + const Symbol *child = verible::DescendThroughSingletons(expr); int value; if (ConstantIntegerValue(*child, &value)) { return value == 0; } if (child->Kind() != SymbolKind::kLeaf) return false; - const auto& term = down_cast(*child); + const auto &term = down_cast(*child); auto text = term.get().text(); // TODO(fangism): Could do more sophisticated constant expression evaluation // but for now this is a good first implementation. return (text == "\'0"); } -bool ConstantIntegerValue(const verible::Symbol& expr, int* value) { - const Symbol* child = verible::DescendThroughSingletons(expr); +bool ConstantIntegerValue(const verible::Symbol &expr, int *value) { + const Symbol *child = verible::DescendThroughSingletons(expr); if (child->Kind() != SymbolKind::kLeaf) return false; - const auto& term = down_cast(*child); + const auto &term = down_cast(*child); // Don't even need to check the leaf token's enumeration type. auto text = term.get().text(); return absl::SimpleAtoi(text, value); } -const verible::Symbol* UnwrapExpression(const verible::Symbol& expr) { +const verible::Symbol *UnwrapExpression(const verible::Symbol &expr) { if (expr.Kind() == SymbolKind::kLeaf) return &expr; - const auto& node = verible::SymbolCastToNode(expr); + const auto &node = verible::SymbolCastToNode(expr); const auto tag = static_cast(node.Tag().tag); if (tag != NodeEnum::kExpression) return &expr; - const auto& children = node.children(); + const auto &children = node.children(); return children.front().get(); } -const verible::Symbol* GetConditionExpressionPredicate( - const verible::Symbol& condition_expr) { +const verible::Symbol *GetConditionExpressionPredicate( + const verible::Symbol &condition_expr) { return GetSubtreeAsSymbol(condition_expr, NodeEnum::kConditionExpression, 0); } -const verible::Symbol* GetConditionExpressionTrueCase( - const verible::Symbol& condition_expr) { +const verible::Symbol *GetConditionExpressionTrueCase( + const verible::Symbol &condition_expr) { return GetSubtreeAsSymbol(condition_expr, NodeEnum::kConditionExpression, 2); } -const verible::Symbol* GetConditionExpressionFalseCase( - const verible::Symbol& condition_expr) { +const verible::Symbol *GetConditionExpressionFalseCase( + const verible::Symbol &condition_expr) { return GetSubtreeAsSymbol(condition_expr, NodeEnum::kConditionExpression, 4); } -const verible::TokenInfo* GetUnaryPrefixOperator( - const verible::Symbol& symbol) { - const SyntaxTreeNode* node = symbol.Kind() == SymbolKind::kNode +const verible::TokenInfo *GetUnaryPrefixOperator( + const verible::Symbol &symbol) { + const SyntaxTreeNode *node = symbol.Kind() == SymbolKind::kNode ? &verible::SymbolCastToNode(symbol) : nullptr; if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) { return nullptr; } - const verible::Symbol* leaf_symbol = node->children().front().get(); - return &verible::down_cast(leaf_symbol) + const verible::Symbol *leaf_symbol = node->children().front().get(); + return &verible::down_cast(leaf_symbol) ->get(); } -const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol& symbol) { - const SyntaxTreeNode* node = symbol.Kind() == SymbolKind::kNode +const verible::Symbol *GetUnaryPrefixOperand(const verible::Symbol &symbol) { + const SyntaxTreeNode *node = symbol.Kind() == SymbolKind::kNode ? &verible::SymbolCastToNode(symbol) : nullptr; if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) { @@ -122,37 +122,37 @@ const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol& symbol) { } std::vector FindAllBinaryOperations( - const verible::Symbol& root) { + const verible::Symbol &root) { return verible::SearchSyntaxTree(root, NodekBinaryExpression()); } std::vector FindAllConditionExpressions( - const verible::Symbol& root) { + const verible::Symbol &root) { return verible::SearchSyntaxTree(root, NodekConditionExpression()); } std::vector FindAllReferenceFullExpressions( - const verible::Symbol& root) { + const verible::Symbol &root) { return verible::SearchSyntaxTree(root, NodekReferenceCallBase()); } -static const verible::TokenInfo* ReferenceBaseIsSimple( - const verible::SyntaxTreeNode& reference_base) { - const Symbol* bottom = verible::DescendThroughSingletons(reference_base); +static const verible::TokenInfo *ReferenceBaseIsSimple( + const verible::SyntaxTreeNode &reference_base) { + const Symbol *bottom = verible::DescendThroughSingletons(reference_base); if (!bottom) return nullptr; const auto tag = bottom->Tag(); if (tag.kind == verible::SymbolKind::kLeaf) { - const auto& token(verible::SymbolCastToLeaf(*bottom).get()); + const auto &token(verible::SymbolCastToLeaf(*bottom).get()); return token.token_enum() == SymbolIdentifier ? &token : nullptr; } // Expect to hit kUnqualifiedId, which has two children. // child[0] should be a SymbolIdentifier (or similar) token. // child[1] are optional #(parameters), which would imply child[0] is // referring to a parameterized type. - const auto& unqualified_id( + const auto &unqualified_id( verible::CheckSymbolAsNode(*bottom, NodeEnum::kUnqualifiedId)); - const auto* params = GetParamListFromUnqualifiedId(unqualified_id); + const auto *params = GetParamListFromUnqualifiedId(unqualified_id); // If there are parameters, it is not simple reference. // It is most likely a class-qualified static reference. return params == nullptr @@ -161,16 +161,16 @@ static const verible::TokenInfo* ReferenceBaseIsSimple( : nullptr; } -const verible::TokenInfo* ReferenceIsSimpleIdentifier( - const verible::Symbol& reference) { - const auto& reference_node( +const verible::TokenInfo *ReferenceIsSimpleIdentifier( + const verible::Symbol &reference) { + const auto &reference_node( verible::CheckSymbolAsNode(reference, NodeEnum::kReferenceCallBase)); // A simple reference contains one component without hierarchy, indexing, or // calls; it looks like just an identifier. if (reference_node.children().size() > 1) return nullptr; - const auto& base_symbol = reference_node.children().front(); + const auto &base_symbol = reference_node.children().front(); if (!base_symbol) return nullptr; - const auto& base_node = verible::SymbolCastToNode(*base_symbol); + const auto &base_node = verible::SymbolCastToNode(*base_symbol); if (!base_node.MatchesTag(NodeEnum::kReference)) return nullptr; return ReferenceBaseIsSimple(base_node); } diff --git a/verilog/CST/expression.h b/verilog/CST/expression.h index 4cf106e60c..e872c354b3 100644 --- a/verilog/CST/expression.h +++ b/verilog/CST/expression.h @@ -32,18 +32,18 @@ namespace verilog { // Example usage: $$ = MakeBinaryExpression($1, $2, $3); template -verible::SymbolPtr MakeBinaryExpression(T1&& lhs, T2&& op, T3&& rhs) { - const verible::SyntaxTreeLeaf& this_op(SymbolCastToLeaf(*op)); +verible::SymbolPtr MakeBinaryExpression(T1 &&lhs, T2 &&op, T3 &&rhs) { + const verible::SyntaxTreeLeaf &this_op(SymbolCastToLeaf(*op)); const auto this_tag = verilog_tokentype(this_op.Tag().tag); // If parent (lhs) operator is associative and matches this one, // then flatten them into the same set of siblings. // This prevents excessive tree depth for long associative expressions. if (IsAssociativeOperator(this_tag) && lhs->Kind() == verible::SymbolKind::kNode) { - const auto& lhs_node = - verible::down_cast(*lhs); + const auto &lhs_node = + verible::down_cast(*lhs); if (lhs_node.MatchesTag(NodeEnum::kBinaryExpression)) { - const verible::SyntaxTreeLeaf& lhs_op = + const verible::SyntaxTreeLeaf &lhs_op = verible::SymbolCastToLeaf(*lhs_node[1]); const auto lhs_tag = verilog_tokentype(lhs_op.Tag().tag); if (lhs_tag == this_tag) { @@ -59,44 +59,44 @@ verible::SymbolPtr MakeBinaryExpression(T1&& lhs, T2&& op, T3&& rhs) { // Returns true if symbol is a kNode tagged with kExpression // Does not match other Expression tags -bool IsExpression(const verible::SymbolPtr&); +bool IsExpression(const verible::SymbolPtr &); // Returns true if expression is a literal 0. // Does not evaluate constant expressions for equivalence to 0. -bool IsZero(const verible::Symbol&); +bool IsZero(const verible::Symbol &); // Returns true if integer value is successfully interpreted. -bool ConstantIntegerValue(const verible::Symbol&, int*); +bool ConstantIntegerValue(const verible::Symbol &, int *); // Returns the Symbol directly underneath a `kExpression` node, otherwise // returns itself. -const verible::Symbol* UnwrapExpression(const verible::Symbol&); +const verible::Symbol *UnwrapExpression(const verible::Symbol &); // Returns the predicate expression of a kConditionExpression. -const verible::Symbol* GetConditionExpressionPredicate(const verible::Symbol&); +const verible::Symbol *GetConditionExpressionPredicate(const verible::Symbol &); // Returns the true-case expression of a kConditionExpression. -const verible::Symbol* GetConditionExpressionTrueCase(const verible::Symbol&); +const verible::Symbol *GetConditionExpressionTrueCase(const verible::Symbol &); // Returns the false-case expression of a kConditionExpression. -const verible::Symbol* GetConditionExpressionFalseCase(const verible::Symbol&); +const verible::Symbol *GetConditionExpressionFalseCase(const verible::Symbol &); // Returns the operator of a kUnaryPrefixExpression -const verible::TokenInfo* GetUnaryPrefixOperator(const verible::Symbol&); +const verible::TokenInfo *GetUnaryPrefixOperator(const verible::Symbol &); // Returns the operand of a kUnaryPrefixExpression -const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol&); +const verible::Symbol *GetUnaryPrefixOperand(const verible::Symbol &); // From binary expression operations, e.g. "a + b". // Associative binary operators may span more than two operands, e.g. "a+b+c". std::vector FindAllBinaryOperations( - const verible::Symbol&); + const verible::Symbol &); // TODO(fangism): evaluate constant expressions // From a statement like "assign foo = condition_a ? a : b;", returns condition // expressions "condition_a ? a : b". std::vector FindAllConditionExpressions( - const verible::Symbol&); + const verible::Symbol &); // TODO(fangism): evaluate constant expressions @@ -105,12 +105,12 @@ std::vector FindAllConditionExpressions( // References include any indexing [], hierarchy ".x" or call "(...)" // extensions. std::vector FindAllReferenceFullExpressions( - const verible::Symbol&); + const verible::Symbol &); // Returns true if reference expression is a plain variable reference with no // hierarchy, no indexing, no calls. -const verible::TokenInfo* ReferenceIsSimpleIdentifier( - const verible::Symbol& reference); +const verible::TokenInfo *ReferenceIsSimpleIdentifier( + const verible::Symbol &reference); } // namespace verilog diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.cc index e4d254bf0b..ee3ed4f518 100644 --- a/verilog/CST/expression_test.cc +++ b/verilog/CST/expression_test.cc @@ -39,7 +39,7 @@ using verible::TextStructureView; using verible::TreeSearchMatch; TEST(IsZeroTest, NonZero) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a", "a0", "1", @@ -53,7 +53,7 @@ TEST(IsZeroTest, NonZero) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -62,7 +62,7 @@ TEST(IsZeroTest, NonZero) { } TEST(IsZeroTest, Zero) { - const char* kTestCases[] = { + const char *kTestCases[] = { "0", "00", "00000", @@ -71,7 +71,7 @@ TEST(IsZeroTest, Zero) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -80,7 +80,7 @@ TEST(IsZeroTest, Zero) { } TEST(ConstantIntegerValueTest, NotInteger) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a", "1+1", "(2)", @@ -88,7 +88,7 @@ TEST(ConstantIntegerValueTest, NotInteger) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -98,7 +98,7 @@ TEST(ConstantIntegerValueTest, NotInteger) { } TEST(ConstantIntegerValueTest, IsInteger) { - const std::pair kTestCases[] = { + const std::pair kTestCases[] = { {"0", 0}, {"1", 1}, {"666", 666}, @@ -106,7 +106,7 @@ TEST(ConstantIntegerValueTest, IsInteger) { for (auto test : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(test.first, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); @@ -135,10 +135,10 @@ TEST(AssociativeBinaryExpressionsTest, FlatTree) { {"function f;\n", "a = ", {kTag, "b ^ c ^ d ^ e"}, "; endfunction\n"}, {"function f;\n", "a = ", {kTag, "b ~^ c ~^ d ~^ e"}, "; endfunction\n"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); return FindAllBinaryOperations(*ABSL_DIE_IF_NULL(root)); }); } @@ -168,12 +168,12 @@ TEST(AssociativeBinaryExpressionsTest, ThreeFlatOperands) { {kTag, "x*y*z"}, "); endfunction\n"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); auto matches = FindAllBinaryOperations(*ABSL_DIE_IF_NULL(root)); - for (const auto& match : matches) { + for (const auto &match : matches) { // "A op B op C" is 5 sibling tokens, due to flattening EXPECT_EQ(verible::SymbolCastToNode(*ABSL_DIE_IF_NULL(match.match)) .children() @@ -225,16 +225,16 @@ TEST(GetConditionExpressionPredicateTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); const auto exprs = FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); std::vector predicates; - for (const auto& expr : exprs) { - const auto* predicate = + for (const auto &expr : exprs) { + const auto *predicate = GetConditionExpressionPredicate(*expr.match); if (predicate != nullptr) { predicates.push_back( @@ -312,16 +312,16 @@ TEST(GetConditionExpressionTrueCaseTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); const auto exprs = FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); std::vector predicates; - for (const auto& expr : exprs) { - const auto* predicate = GetConditionExpressionTrueCase(*expr.match); + for (const auto &expr : exprs) { + const auto *predicate = GetConditionExpressionTrueCase(*expr.match); if (predicate != nullptr) { predicates.push_back( TreeSearchMatch{predicate, {/* ignored context */}}); @@ -398,16 +398,16 @@ TEST(GetConditionExpressionFalseCaseTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); const auto exprs = FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); std::vector predicates; - for (const auto& expr : exprs) { - const auto* predicate = + for (const auto &expr : exprs) { + const auto *predicate = GetConditionExpressionFalseCase(*expr.match); if (predicate != nullptr) { predicates.push_back( @@ -424,21 +424,21 @@ TEST(GetConditionExpressionFalseCaseTest, Various) { } TEST(GetUnaryPrefixOperator, Exprs) { - const std::pair kTestCases[] = { + const std::pair kTestCases[] = { {"-(2)", "-"}, {"-1", "-"}, {"&1", "&"}, {"666", nullptr}, {"1 + 2", nullptr}, {"!1", "!"}, }; for (auto test : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(test.first, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); - const verible::Symbol* last_node = DescendThroughSingletons(*node); + const verible::Symbol *last_node = DescendThroughSingletons(*node); if (test.second) { - const verible::SyntaxTreeNode& unary_expr = + const verible::SyntaxTreeNode &unary_expr = verible::SymbolCastToNode(*last_node); EXPECT_EQ(NodeEnum(unary_expr.Tag().tag), NodeEnum::kUnaryPrefixExpression); @@ -452,20 +452,20 @@ TEST(GetUnaryPrefixOperator, Exprs) { } TEST(GetUnaryPrefixOperand, Exprs) { - const std::pair kTestCases[] = { + const std::pair kTestCases[] = { {"-1", ""}, {"&1", ""}, {"666", nullptr}, {"1 + 2", nullptr}, {"!1", ""}, }; for (auto test : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(test.first, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); const auto tag = node->Tag(); EXPECT_EQ(tag.kind, verible::SymbolKind::kNode); EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression); - const verible::Symbol* last_node = DescendThroughSingletons(*node); + const verible::Symbol *last_node = DescendThroughSingletons(*node); if (test.second) { - const verible::SyntaxTreeNode& unary_expr = + const verible::SyntaxTreeNode &unary_expr = verible::SymbolCastToNode(*last_node); EXPECT_EQ(NodeEnum(unary_expr.Tag().tag), NodeEnum::kUnaryPrefixExpression); @@ -513,10 +513,10 @@ TEST(FindAllConditionExpressionsTest, Various) { "endcase\n", "\nendmodule"}, }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); return FindAllConditionExpressions(*ABSL_DIE_IF_NULL(root)); }); } @@ -626,17 +626,17 @@ TEST(FindAllReferenceExpressionsTest, Various) { // reference could contain other references like "a[a]", but the testing // framework doesn't support nested expected ranges... yet. }; - for (const auto& test : kTestCases) { + for (const auto &test : kTestCases) { TestVerilogSyntaxRangeMatches( - __FUNCTION__, test, [](const TextStructureView& text_structure) { - const auto& root = text_structure.SyntaxTree(); + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); return FindAllReferenceFullExpressions(*ABSL_DIE_IF_NULL(root)); }); } } TEST(ReferenceIsSimpleTest, Simple) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a", "bbb", "z1", @@ -645,7 +645,7 @@ TEST(ReferenceIsSimpleTest, Simple) { for (auto code : kTestCases) { const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); { const auto status = analyzer_ptr->LexStatus(); ASSERT_TRUE(status.ok()) << status.message(); @@ -656,8 +656,8 @@ TEST(ReferenceIsSimpleTest, Simple) { } const std::vector refs( FindAllReferenceFullExpressions(*ABSL_DIE_IF_NULL(node))); - for (const auto& ref : refs) { - const verible::TokenInfo* token = ReferenceIsSimpleIdentifier(*ref.match); + for (const auto &ref : refs) { + const verible::TokenInfo *token = ReferenceIsSimpleIdentifier(*ref.match); ASSERT_NE(token, nullptr) << "reference: " << code; EXPECT_EQ(token->text(), code); } @@ -665,7 +665,7 @@ TEST(ReferenceIsSimpleTest, Simple) { } TEST(ReferenceIsSimpleTest, NotSimple) { - const char* kTestCases[] = { + const char *kTestCases[] = { "a[0]", "a[4][6]", "bbb[3:0]", "x.y", "x.y.z", "x[0].y[1].z[2]", "x()", "x.y()", "x()[0]", "x(1)", "f(0,1)", "j[9].k(3, 2, 1)", }; @@ -673,7 +673,7 @@ TEST(ReferenceIsSimpleTest, NotSimple) { VLOG(1) << __FUNCTION__ << " test: " << code; const auto analyzer_ptr = AnalyzeVerilogExpression(code, "", kDefaultPreprocess); - const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); { const auto status = analyzer_ptr->LexStatus(); ASSERT_TRUE(status.ok()) << status.message(); @@ -684,7 +684,7 @@ TEST(ReferenceIsSimpleTest, NotSimple) { } const std::vector refs( FindAllReferenceFullExpressions(*ABSL_DIE_IF_NULL(node))); - for (const auto& ref : refs) { + for (const auto &ref : refs) { VLOG(1) << "match: " << verible::StringSpanOfSymbol(*ref.match); EXPECT_FALSE(ReferenceIsSimpleIdentifier(*ref.match)) << "reference: " << code; From 87a7b238c0db85ff1368cf0b1b17eaf7a2952e09 Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Sun, 7 May 2023 20:21:38 +0200 Subject: [PATCH 2/7] Add getters for IncrementDecrementExpression --- verilog/CST/expression.cc | 37 +++++++++++++++++ verilog/CST/expression.h | 10 +++++ verilog/CST/expression_test.cc | 75 ++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+) diff --git a/verilog/CST/expression.cc b/verilog/CST/expression.cc index 88eb35dac0..2af6ca52a3 100644 --- a/verilog/CST/expression.cc +++ b/verilog/CST/expression.cc @@ -175,4 +175,41 @@ const verible::TokenInfo *ReferenceIsSimpleIdentifier( return ReferenceBaseIsSimple(base_node); } +const verible::SyntaxTreeLeaf *GetIncrementDecrementOperator( + const verible::Symbol &expr) { + if (expr.Kind() != SymbolKind::kNode) return nullptr; + + const SyntaxTreeNode &node = verible::SymbolCastToNode(expr); + + if (!node.MatchesTag(NodeEnum::kIncrementDecrementExpression)) return nullptr; + + // Structure changes depending on the type of IncrementDecrement + bool is_post = node.children().front().get()->Kind() == SymbolKind::kNode; + + if (is_post) { + return verible::GetSubtreeAsLeaf( + expr, NodeEnum::kIncrementDecrementExpression, 1); + } + return verible::GetSubtreeAsLeaf(expr, + NodeEnum::kIncrementDecrementExpression, 0); +} + +const verible::SyntaxTreeNode *GetIncrementDecrementOperand( + const verible::Symbol &expr) { + if (expr.Kind() != SymbolKind::kNode) return nullptr; + + const SyntaxTreeNode &node = verible::SymbolCastToNode(expr); + + if (!node.MatchesTag(NodeEnum::kIncrementDecrementExpression)) return nullptr; + + // Structure changes depending on the type of IncrementDecrement + bool is_post = node.children().front().get()->Kind() == SymbolKind::kNode; + if (is_post) { + return verible::GetSubtreeAsNode( + expr, NodeEnum::kIncrementDecrementExpression, 0); + } + return verible::GetSubtreeAsNode(expr, + NodeEnum::kIncrementDecrementExpression, 1); +} + } // namespace verilog diff --git a/verilog/CST/expression.h b/verilog/CST/expression.h index e872c354b3..b10eea4e64 100644 --- a/verilog/CST/expression.h +++ b/verilog/CST/expression.h @@ -112,6 +112,16 @@ std::vector FindAllReferenceFullExpressions( const verible::TokenInfo *ReferenceIsSimpleIdentifier( const verible::Symbol &reference); +// Return the operator for a kIncrementDecrementExpression +// This function would extract the leaf containing '++' from expression '++a' +const verible::SyntaxTreeLeaf *GetIncrementDecrementOperator( + const verible::Symbol &expr); + +// Return the operator for a kIncrementDecrementExpression +// This function would extract the leaf containing 'a' from expression '++a' +const verible::SyntaxTreeNode *GetIncrementDecrementOperand( + const verible::Symbol &expr); + } // namespace verilog #endif // VERIBLE_VERILOG_CST_EXPRESSION_H_ diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.cc index ee3ed4f518..eb42794c3a 100644 --- a/verilog/CST/expression_test.cc +++ b/verilog/CST/expression_test.cc @@ -692,5 +692,80 @@ TEST(ReferenceIsSimpleTest, NotSimple) { } } +TEST(GetIncrementDecrementOperatorTest, Various) { + const std::pair kTestCases[] = { + {"a++", "++"}, + {"++a", "++"}, + {"somelargename++", "++"}, + {"++somelargename", "++"}, + {"a+1", ""}, + {"1", ""}, + }; + + for (auto testcase : kTestCases) { + VLOG(1) << __FUNCTION__ << " test: " << testcase.first; + const auto analyzer_ptr = + AnalyzeVerilogExpression(testcase.first, "", kDefaultPreprocess); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + { + const auto status = analyzer_ptr->LexStatus(); + ASSERT_TRUE(status.ok()) << status.message(); + } + { + const auto status = analyzer_ptr->ParseStatus(); + ASSERT_TRUE(status.ok()) << status.message(); + } + const verible::Symbol &symbol = *ABSL_DIE_IF_NULL(node); + + // We have to get to the actual kIncrementDecrementExpression, + // node points to kExpression + const verible::Symbol &incr_decr_expr = *UnwrapExpression(symbol); + const verible::SyntaxTreeLeaf *operator_ = + GetIncrementDecrementOperator(incr_decr_expr); + + const absl::string_view string_span = + operator_ ? StringSpanOfSymbol(*operator_) : ""; + + EXPECT_EQ(string_span, testcase.second); + } +} + +TEST(GetIncrementDecrementOperandTest, Various) { + const std::pair kTestCases[] = { + {"a++", "a"}, + {"++a", "a"}, + {"somelargename++", "somelargename"}, + {"++somelargename", "somelargename"}, + {"a+1", ""}, + {"1", ""}, + }; + + for (auto testcase : kTestCases) { + VLOG(1) << __FUNCTION__ << " test: " << testcase.first; + const auto analyzer_ptr = + AnalyzeVerilogExpression(testcase.first, "", kDefaultPreprocess); + const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); + { + const auto status = analyzer_ptr->LexStatus(); + ASSERT_TRUE(status.ok()) << status.message(); + } + { + const auto status = analyzer_ptr->ParseStatus(); + ASSERT_TRUE(status.ok()) << status.message(); + } + const verible::Symbol &symbol = *ABSL_DIE_IF_NULL(node); + + // We have to get to the actual kIncrementDecrementExpression, + // node points to kExpression + const verible::Symbol &incr_decr_expr = *UnwrapExpression(symbol); + const verible::SyntaxTreeNode *operand = + GetIncrementDecrementOperand(incr_decr_expr); + const absl::string_view string_span = + operand ? StringSpanOfSymbol(*operand) : ""; + + EXPECT_EQ(string_span, testcase.second); + } +} + } // namespace } // namespace verilog From 8b12538e9b964f5953d866d77ab82f24a6ed3779 Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Sun, 7 May 2023 20:45:48 +0200 Subject: [PATCH 3/7] Add getters for AssignModifyStatements and NetVariableAssignment --- verilog/CST/statement.cc | 40 +++++++ verilog/CST/statement.h | 20 ++++ verilog/CST/statement_test.cc | 208 ++++++++++++++++++++++++++++++++++ 3 files changed, 268 insertions(+) diff --git a/verilog/CST/statement.cc b/verilog/CST/statement.cc index b6986ee690..6d56f4d260 100644 --- a/verilog/CST/statement.cc +++ b/verilog/CST/statement.cc @@ -501,4 +501,44 @@ const verible::Symbol *GetEventControlFromProceduralTimingControl( 0, NodeEnum::kEventControl); } +const verible::SyntaxTreeLeaf *GetAssignModifyOperator( + const verible::SyntaxTreeNode &assign_modify) { + if (!assign_modify.MatchesTag(NodeEnum::kAssignModifyStatement)) { + return nullptr; + } + + return verible::GetSubtreeAsLeaf(assign_modify, + NodeEnum::kAssignModifyStatement, 1); +} + +const verible::SyntaxTreeNode *GetAssignModifyRhs( + const verible::SyntaxTreeNode &assign_modify) { + if (!assign_modify.MatchesTag(NodeEnum::kAssignModifyStatement)) { + return nullptr; + } + + return verible::GetSubtreeAsNode(assign_modify, + NodeEnum::kAssignModifyStatement, 2); +} + +const verible::SyntaxTreeNode *GetAssignModifyLhs( + const verible::SyntaxTreeNode &assign_modify) { + if (!assign_modify.MatchesTag(NodeEnum::kAssignModifyStatement)) { + return nullptr; + } + + return verible::GetSubtreeAsNode(assign_modify, + NodeEnum::kAssignModifyStatement, 0); +} + +const verible::SyntaxTreeNode *GetNetVariableAssignmentLhs( + const verible::SyntaxTreeNode &assignment) { + if (!assignment.MatchesTag(NodeEnum::kNetVariableAssignment)) { + return nullptr; + } + + return verible::GetSubtreeAsNode(assignment, NodeEnum::kNetVariableAssignment, + 0); +} + } // namespace verilog diff --git a/verilog/CST/statement.h b/verilog/CST/statement.h index a6811a9a04..a437ab7dea 100644 --- a/verilog/CST/statement.h +++ b/verilog/CST/statement.h @@ -202,6 +202,26 @@ const verible::SyntaxTreeNode *GetProceduralTimingControlFromAlways( const verible::Symbol *GetEventControlFromProceduralTimingControl( const verible::SyntaxTreeNode &proc_timing_ctrl); +// Return the operator from an AssignModifyStatement +// Example: get '&' from 'x &= y' +const verible::SyntaxTreeLeaf *GetAssignModifyOperator( + const verible::SyntaxTreeNode &assign_modify); + +// Return the right hand side (Rhs) from an AssignModifyStatement +// Example: get 'y' from 'x &= y' +const verible::SyntaxTreeNode *GetAssignModifyRhs( + const verible::SyntaxTreeNode &assign_modify); + +// Return the left hand side (Lhs) from an AssignModifyStatement +// Example: get 'x' from 'x &= y' +const verible::SyntaxTreeNode *GetAssignModifyLhs( + const verible::SyntaxTreeNode &assign_modify); + +// Return the left hand side (Lhs) from a NetVariableAssignment +// Example: get 'x' from 'x = y' +const verible::SyntaxTreeNode *GetNetVariableAssignmentLhs( + const verible::SyntaxTreeNode &assignment); + } // namespace verilog #endif // VERIBLE_VERILOG_CST_STATEMENT_H_ diff --git a/verilog/CST/statement_test.cc b/verilog/CST/statement_test.cc index 3447778e70..3a308cdf63 100644 --- a/verilog/CST/statement_test.cc +++ b/verilog/CST/statement_test.cc @@ -1373,5 +1373,213 @@ TEST(GetGenerateBlockEndTest, Various) { } } +TEST(GetAssignModifyOperator, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k ", + {kTag, "&="}, + " 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k ", + {kTag, "|="}, + " 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k ", + {kTag, "+="}, + " 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "always_comb begin " + "reg k = 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), + NodekAssignModifyStatement()); + + std::vector ends; + for (const auto &block : blocks) { + const auto *end = GetAssignModifyOperator( + verible::SymbolCastToNode(*block.match)); + ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + } + return ends; + }); + } +} + +TEST(GetAssignModifyLhs, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin ", + {kTag, "k"}, + " &= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " |= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " += 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "always_comb begin\n" + "reg k = 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), + NodekAssignModifyStatement()); + + std::vector ends; + for (const auto &block : blocks) { + const auto *end = + GetAssignModifyLhs(verible::SymbolCastToNode(*block.match)); + ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + } + return ends; + }); + } +} + +TEST(GetAssignModifyRhs, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin " + "k &= ", + {kTag, "1"}, + ";\nend\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k |= ", + {kTag, "(1 + 2)"}, + ";\nend\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k += ", + {kTag, "1"}, + ";\nend\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "always_comb begin\n" + "reg k = 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), + NodekAssignModifyStatement()); + + std::vector ends; + for (const auto &block : blocks) { + const auto *end = + GetAssignModifyRhs(verible::SymbolCastToNode(*block.match)); + ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + } + return ends; + }); + } +} + +TEST(GetNetVariableAssignmentLhs, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin ", + {kTag, "k"}, + " = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k |= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_ff begin\n" + "k <= 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), + NodekNetVariableAssignment()); + + std::vector ends; + for (const auto &block : blocks) { + const auto *end = GetNetVariableAssignmentLhs( + verible::SymbolCastToNode(*block.match)); + ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + } + return ends; + }); + } +} + } // namespace } // namespace verilog From 00fea040b42c12e3d492055364e506ea763647f6 Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Sun, 7 May 2023 20:54:39 +0200 Subject: [PATCH 4/7] Add autofixes for always_ff_non_blocking_rule --- verilog/analysis/checkers/BUILD | 2 + .../checkers/always_ff_non_blocking_rule.cc | 109 +++++++++++++----- .../always_ff_non_blocking_rule_test.cc | 51 ++++++++ 3 files changed, 130 insertions(+), 32 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 27b425f36b..729a6c16a5 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1204,6 +1204,8 @@ cc_library( "//common/text:symbol", "//common/text:syntax-tree-context", "//common/util:casts", + "//verilog/CST:expression", + "//verilog/CST:statement", "//verilog/CST:verilog-matchers", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc index ce61e47fda..dddf214319 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -27,6 +27,8 @@ #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" #include "common/util/casts.h" +#include "verilog/CST/expression.h" +#include "verilog/CST/statement.h" #include "verilog/CST/verilog_matchers.h" // IWYU pragma: keep #include "verilog/analysis/descriptions.h" #include "verilog/analysis/lint_rule_registry.h" @@ -34,6 +36,7 @@ namespace verilog { namespace analysis { +using verible::AutoFix; using verible::LintRuleStatus; using verible::LintViolation; using verible::SearchSyntaxTree; @@ -48,7 +51,7 @@ static constexpr absl::string_view kMessage = "Use blocking assignments, at most, for locals inside " "'always_ff' sequential blocks."; -const LintRuleDescriptor& AlwaysFFNonBlockingRule::GetDescriptor() { +const LintRuleDescriptor &AlwaysFFNonBlockingRule::GetDescriptor() { static const LintRuleDescriptor d{ .name = "always-ff-non-blocking", .topic = "sequential-logic", @@ -78,8 +81,8 @@ absl::Status AlwaysFFNonBlockingRule::Configure( } //- Processing -------------------------------------------------------------- -void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol& symbol, - const SyntaxTreeContext& context) { +void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, + const SyntaxTreeContext &context) { //- Process and filter context before locating blocking assigments -------- // Detect entering and leaving of always_ff blocks @@ -97,31 +100,71 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol& symbol, static const Matcher asgn_incdec_matcher{NodekIncrementDecrementExpression()}; static const Matcher ident_matcher{NodekUnqualifiedId()}; + std::vector autofixes; + // Rule may be waived if complete lhs consists of local variables // -> determine root of lhs - const verible::Symbol* check_root = nullptr; + const verible::Symbol *check_root = nullptr; verible::matcher::BoundSymbolManager symbol_man; if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) { - if (const auto* const node = - verible::down_cast(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - node->children()[0].get()); - } + const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol); + check_root = verilog::GetNetVariableAssignmentLhs(*node); + + const verible::SyntaxTreeLeaf *leaf = + verible::GetSubtreeAsLeaf(symbol, NodeEnum::kNetVariableAssignment, 1); + + autofixes.emplace_back(AutoFix( + "Substitute blocking assignment '=' for nonblocking assignment '<='", + {leaf->get(), "<="})); } else { // Not interested in any other blocking assignments unless flagged if (!catch_modifying_assignments_) return; + // These autofixes require substituting the whole expression + absl::string_view original = verible::StringSpanOfSymbol(symbol); if (asgn_modify_matcher.Matches(symbol, &symbol_man)) { - if (const auto* const node = - verible::down_cast(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - node->children()[0].get()); - } + const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol); + const verible::SyntaxTreeNode &lhs = *GetAssignModifyLhs(*node); + const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node); + const verible::SyntaxTreeLeaf &operator_ = + *GetAssignModifyOperator(*node); + + check_root = &lhs; + + const std::string fix = + absl::StrCat(verible::StringSpanOfSymbol(lhs), + " <= ", verible::StringSpanOfSymbol(lhs), " ", + verible::StringSpanOfSymbol(operator_).substr(0, 1), " ", + verible::StringSpanOfSymbol(rhs), ";"); + + autofixes.emplace_back( + AutoFix("Substitute assignment operator for equivalent " + "nonblocking assignment", + {original, fix})); } else if (asgn_incdec_matcher.Matches(symbol, &symbol_man)) { - check_root = &symbol; + const verible::SyntaxTreeNode *operand = + GetIncrementDecrementOperand(symbol); + const verible::SyntaxTreeLeaf *operator_ = + GetIncrementDecrementOperator(symbol); + check_root = operand; + + const absl::string_view identifier = + verible::StringSpanOfSymbol(*operand); + // Extract just the operation. Just '+' from '++' + const absl::string_view op = + verible::StringSpanOfSymbol(*operator_).substr(0, 1); + + // Equivalent nonblocking assignment + // {'x++', '++x'} become 'x <= x + 1' + // {'x--', '--x'} become 'x <= x - 1' + std::string fix = + absl::StrCat(identifier, " <= ", identifier, " ", op, " 1;"); + + autofixes.emplace_back( + AutoFix("Substitute increment/decrement operator for " + "equivalent nonblocking assignment", + {original, fix})); } else { // Not a blocking assignment return; @@ -133,16 +176,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol& symbol, bool waived = false; if (waive_for_locals_ && check_root) { waived = true; - for (const auto& var : SearchSyntaxTree(*check_root, ident_matcher)) { + for (const auto &var : SearchSyntaxTree(*check_root, ident_matcher)) { if (var.context.IsInside(NodeEnum::kDimensionScalar)) continue; if (var.context.IsInside(NodeEnum::kDimensionSlice)) continue; if (var.context.IsInside(NodeEnum::kHierarchyExtension)) continue; bool found = false; - if (const auto* const varn = - verible::down_cast(var.match)) { - if (const auto* const ident = - verible::down_cast( + if (const auto *const varn = + verible::down_cast(var.match)) { + if (const auto *const ident = + verible::down_cast( varn->children()[0].get())) { found = std::find(locals_.begin(), locals_.end(), ident->get().text()) != locals_.end(); @@ -155,10 +198,12 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol& symbol, } // Enqueue detected violation unless waived - if (!waived) violations_.insert(LintViolation(symbol, kMessage, context)); + if (!waived) { + violations_.insert(LintViolation(symbol, kMessage, context, autofixes)); + } } // HandleSymbol() -bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol& symbol, +bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol, const int depth) { static const Matcher always_ff_matcher{ NodekAlwaysStatement(AlwaysFFKeyword())}; @@ -197,18 +242,18 @@ bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol& symbol, return true; } // InsideBlock() -bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol& symbol) { +bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) { static const Matcher decl_matcher{NodekDataDeclaration()}; static const Matcher var_matcher{NodekRegisterVariable()}; verible::matcher::BoundSymbolManager symbol_man; if (decl_matcher.Matches(symbol, &symbol_man)) { - auto& count = scopes_.top().inherited_local_count; - for (const auto& var : SearchSyntaxTree(symbol, var_matcher)) { - if (const auto* const node = - verible::down_cast(var.match)) { - if (const auto* const ident = - verible::down_cast( + auto &count = scopes_.top().inherited_local_count; + for (const auto &var : SearchSyntaxTree(symbol, var_matcher)) { + if (const auto *const node = + verible::down_cast(var.match)) { + if (const auto *const ident = + verible::down_cast( node->children()[0].get())) { const absl::string_view name = ident->get().text(); VLOG(4) << "Registering '" << name << '\'' << std::endl; diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc index cec73323c8..0b65864800 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc @@ -30,6 +30,7 @@ namespace analysis { namespace { using verible::LintTestCase; +using verible::RunApplyFixCases; using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; @@ -120,6 +121,15 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { {kToken, "a"}, "= b;\n" "end\nend\nendmodule"}, + // Waive, 'a' is a local declaration + {"module m;\nalways_ff @(posedge c) begin static type(b) a; a = b; " + "end\nendmodule"}, + {"module m;\nalways_ff @(posedge c) begin static type(b) a; a++; " + "end\nendmodule"}, + {"module m;\nalways_ff @(posedge c) begin static type(b) a; ++a; " + "end\nendmodule"}, + {"module m;\nalways_ff @(posedge c) begin static type(b) a; a &= b; " + "end\nendmodule"}, }; RunConfiguredLintTestCases( @@ -127,6 +137,47 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { "catch_modifying_assignments:on;waive_for_locals:on"); } +TEST(AlwaysFFNonBlockingRule, AutoFixDefault) { + const std::initializer_list kTestCases = { + // Check we're not waiving local variables + {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", + "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, + {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", + "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, + // Check we're ignoring modifying assignments + {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", + "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, + {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", + "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, + }; + + RunApplyFixCases(kTestCases, ""); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) { + const std::initializer_list kTestCases = { + {"module m;\nalways_ff begin k &= 1; end\nendmodule", + "module m;\nalways_ff begin k <= k & 1; end\nendmodule"}, + {"module m;\nalways_ff begin k |= (2 + 1); end\nendmodule", + "module m;\nalways_ff begin k <= k | (2 + 1); end\nendmodule"}, + }; + + RunApplyFixCases( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixWaiveLocals) { + const std::initializer_list kTestCases = { + // Check that we're correctly waiving the 'p = 0' as it is a local + // variable + {"module m;\nalways_ff begin reg p; p = 0; k = 1; end\nendmodule", + "module m;\nalways_ff begin reg p; p = 0; k <= 1; end\nendmodule"}, + }; + + RunApplyFixCases( + kTestCases, "waive_for_locals:on"); +} + } // namespace } // namespace analysis } // namespace verilog From 3e35d35865dedf1500995d528e8ed3733657ab66 Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Tue, 9 May 2023 08:19:17 +0200 Subject: [PATCH 5/7] fixup! Add getters for IncrementDecrementExpression --- verilog/CST/expression_test.cc | 140 ++++++++++++++++++--------------- 1 file changed, 77 insertions(+), 63 deletions(-) diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.cc index eb42794c3a..6b6b46d114 100644 --- a/verilog/CST/expression_test.cc +++ b/verilog/CST/expression_test.cc @@ -24,6 +24,7 @@ #include "common/util/logging.h" #include "gtest/gtest.h" #include "verilog/CST/match_test_utils.h" +#include "verilog/CST/verilog_matchers.h" #include "verilog/CST/verilog_nonterminals.h" #include "verilog/CST/verilog_tree_print.h" #include "verilog/analysis/verilog_analyzer.h" @@ -693,77 +694,90 @@ TEST(ReferenceIsSimpleTest, NotSimple) { } TEST(GetIncrementDecrementOperatorTest, Various) { - const std::pair kTestCases[] = { - {"a++", "++"}, - {"++a", "++"}, - {"somelargename++", "++"}, - {"++somelargename", "++"}, - {"a+1", ""}, - {"1", ""}, + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m; endmodule\n"}, + {"module m;\ninitial begin end\nendmodule"}, + {"module m;\nalways_comb begin\n" + "a", + {kTag, "++"}, + ";\nend\nendmodule"}, + {"module m;\nalways_comb begin\n", + {kTag, "++"}, + "a;" + "\nend\nendmodule"}, + {"module m;\nalways_comb begin\n" + "somelargename", + {kTag, "++"}, + ";\nend\nendmodule"}, + {"module m;\nalways_comb begin\n", + {kTag, "++"}, + "somelargename;\n" + "end\nendmodule"}, + {"module m;\nalways_comb begin\nk = a + 2;\nend\nendmodule"}, }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto exprs = verible::SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekIncrementDecrementExpression()); - for (auto testcase : kTestCases) { - VLOG(1) << __FUNCTION__ << " test: " << testcase.first; - const auto analyzer_ptr = - AnalyzeVerilogExpression(testcase.first, "", kDefaultPreprocess); - const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); - { - const auto status = analyzer_ptr->LexStatus(); - ASSERT_TRUE(status.ok()) << status.message(); - } - { - const auto status = analyzer_ptr->ParseStatus(); - ASSERT_TRUE(status.ok()) << status.message(); - } - const verible::Symbol &symbol = *ABSL_DIE_IF_NULL(node); - - // We have to get to the actual kIncrementDecrementExpression, - // node points to kExpression - const verible::Symbol &incr_decr_expr = *UnwrapExpression(symbol); - const verible::SyntaxTreeLeaf *operator_ = - GetIncrementDecrementOperator(incr_decr_expr); - - const absl::string_view string_span = - operator_ ? StringSpanOfSymbol(*operator_) : ""; - - EXPECT_EQ(string_span, testcase.second); + std::vector operators; + for (const auto &expr : exprs) { + const auto *operator_ = GetIncrementDecrementOperator(*expr.match); + operators.push_back( + TreeSearchMatch{operator_, {/* ignored context */}}); + } + return operators; + }); } } TEST(GetIncrementDecrementOperandTest, Various) { - const std::pair kTestCases[] = { - {"a++", "a"}, - {"++a", "a"}, - {"somelargename++", "somelargename"}, - {"++somelargename", "somelargename"}, - {"a+1", ""}, - {"1", ""}, + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m; endmodule\n"}, + {"module m;\ninitial begin end\nendmodule"}, + {"module m;\n" + "always_comb begin\n", + {kTag, "a"}, + "++;\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n" + "++", + {kTag, "a"}, + ";\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n", + {kTag, "somelargename"}, + "++;\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n++", + {kTag, "somelargename"}, + ";\nend\nendmodule"}, + {"module m;\n" + "always_comb begin\n" + "k = a + 2;\n" + "end\nendmodule"}, }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto exprs = verible::SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekIncrementDecrementExpression()); - for (auto testcase : kTestCases) { - VLOG(1) << __FUNCTION__ << " test: " << testcase.first; - const auto analyzer_ptr = - AnalyzeVerilogExpression(testcase.first, "", kDefaultPreprocess); - const auto &node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree(); - { - const auto status = analyzer_ptr->LexStatus(); - ASSERT_TRUE(status.ok()) << status.message(); - } - { - const auto status = analyzer_ptr->ParseStatus(); - ASSERT_TRUE(status.ok()) << status.message(); - } - const verible::Symbol &symbol = *ABSL_DIE_IF_NULL(node); - - // We have to get to the actual kIncrementDecrementExpression, - // node points to kExpression - const verible::Symbol &incr_decr_expr = *UnwrapExpression(symbol); - const verible::SyntaxTreeNode *operand = - GetIncrementDecrementOperand(incr_decr_expr); - const absl::string_view string_span = - operand ? StringSpanOfSymbol(*operand) : ""; - - EXPECT_EQ(string_span, testcase.second); + std::vector operands; + for (const auto &expr : exprs) { + const auto *operand = GetIncrementDecrementOperand(*expr.match); + operands.push_back( + TreeSearchMatch{operand, {/* ignored context */}}); + } + return operands; + }); } } From de5d57bc40fa25d67e4b8984626e1fe956df9075 Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Tue, 9 May 2023 08:29:54 +0200 Subject: [PATCH 6/7] fixup! Add getters for AssignModifyStatements and NetVariableAssignment --- verilog/CST/statement_test.cc | 68 ++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/verilog/CST/statement_test.cc b/verilog/CST/statement_test.cc index 3a308cdf63..ab4e08c414 100644 --- a/verilog/CST/statement_test.cc +++ b/verilog/CST/statement_test.cc @@ -1413,16 +1413,17 @@ TEST(GetAssignModifyOperator, Various) { TestVerilogSyntaxRangeMatches( __FUNCTION__, test, [](const TextStructureView &text_structure) { const auto &root = text_structure.SyntaxTree(); - const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), - NodekAssignModifyStatement()); + const auto &assign_modify_statements = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekAssignModifyStatement()); - std::vector ends; - for (const auto &block : blocks) { - const auto *end = GetAssignModifyOperator( + std::vector operators; + for (const auto &block : assign_modify_statements) { + const auto *operator_ = GetAssignModifyOperator( verible::SymbolCastToNode(*block.match)); - ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + operators.emplace_back( + TreeSearchMatch{operator_, {/* ignored context */}}); } - return ends; + return operators; }); } } @@ -1464,16 +1465,17 @@ TEST(GetAssignModifyLhs, Various) { TestVerilogSyntaxRangeMatches( __FUNCTION__, test, [](const TextStructureView &text_structure) { const auto &root = text_structure.SyntaxTree(); - const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), - NodekAssignModifyStatement()); + const auto &assign_modify_statements = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekAssignModifyStatement()); - std::vector ends; - for (const auto &block : blocks) { - const auto *end = + std::vector left_hand_sides; + for (const auto &block : assign_modify_statements) { + const auto *lhs = GetAssignModifyLhs(verible::SymbolCastToNode(*block.match)); - ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + left_hand_sides.emplace_back( + TreeSearchMatch{lhs, {/* ignored context */}}); } - return ends; + return left_hand_sides; }); } } @@ -1515,16 +1517,17 @@ TEST(GetAssignModifyRhs, Various) { TestVerilogSyntaxRangeMatches( __FUNCTION__, test, [](const TextStructureView &text_structure) { const auto &root = text_structure.SyntaxTree(); - const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), - NodekAssignModifyStatement()); + const auto &assign_modify_statements = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekAssignModifyStatement()); - std::vector ends; - for (const auto &block : blocks) { - const auto *end = + std::vector right_hand_sides; + for (const auto &block : assign_modify_statements) { + const auto *rhs = GetAssignModifyRhs(verible::SymbolCastToNode(*block.match)); - ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + right_hand_sides.emplace_back( + TreeSearchMatch{rhs, {/* ignored context */}}); } - return ends; + return right_hand_sides; }); } } @@ -1536,9 +1539,9 @@ TEST(GetNetVariableAssignmentLhs, Various) { {"module m;\nendmodule\n"}, {"module m;\n" "reg k;\n" - "always_comb begin ", + "always_comb begin\n", {kTag, "k"}, - " = 1;\nend\n" + " = 1;\nend\n", "endmodule\n"}, {"module m;\n" "reg k;\n" @@ -1567,16 +1570,17 @@ TEST(GetNetVariableAssignmentLhs, Various) { TestVerilogSyntaxRangeMatches( __FUNCTION__, test, [](const TextStructureView &text_structure) { const auto &root = text_structure.SyntaxTree(); - const auto &blocks = SearchSyntaxTree(*ABSL_DIE_IF_NULL(root), - NodekNetVariableAssignment()); - - std::vector ends; - for (const auto &block : blocks) { - const auto *end = GetNetVariableAssignmentLhs( - verible::SymbolCastToNode(*block.match)); - ends.emplace_back(TreeSearchMatch{end, {/* ignored context */}}); + const auto &net_var_assignments = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekNetVariableAssignment()); + + std::vector left_hand_sides; + for (const auto &assignment : net_var_assignments) { + const auto *lhs = GetNetVariableAssignmentLhs( + verible::SymbolCastToNode(*assignment.match)); + left_hand_sides.emplace_back( + TreeSearchMatch{lhs, {/* ignored context */}}); } - return ends; + return left_hand_sides; }); } } From 6f5efe8b722c4e8c83b6daa74a3c971a1adaf8bd Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Sat, 20 May 2023 09:55:55 +0200 Subject: [PATCH 7/7] fixup! Add autofixes for always_ff_non_blocking_rule --- .../checkers/always_ff_non_blocking_rule.cc | 120 ++++++++++++++++-- .../checkers/always_ff_non_blocking_rule.h | 34 ++++- .../always_ff_non_blocking_rule_test.cc | 36 +++++- 3 files changed, 168 insertions(+), 22 deletions(-) diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc index dddf214319..7191ce81e7 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc @@ -17,6 +17,7 @@ #include #include +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" @@ -105,11 +106,13 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, // Rule may be waived if complete lhs consists of local variables // -> determine root of lhs const verible::Symbol *check_root = nullptr; + absl::string_view lhs_id; verible::matcher::BoundSymbolManager symbol_man; if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) { const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol); check_root = verilog::GetNetVariableAssignmentLhs(*node); + lhs_id = verible::StringSpanOfSymbol(*check_root); const verible::SyntaxTreeLeaf *leaf = verible::GetSubtreeAsLeaf(symbol, NodeEnum::kNetVariableAssignment, 1); @@ -129,14 +132,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node); const verible::SyntaxTreeLeaf &operator_ = *GetAssignModifyOperator(*node); - check_root = &lhs; - const std::string fix = - absl::StrCat(verible::StringSpanOfSymbol(lhs), - " <= ", verible::StringSpanOfSymbol(lhs), " ", - verible::StringSpanOfSymbol(operator_).substr(0, 1), " ", - verible::StringSpanOfSymbol(rhs), ";"); + bool needs_parenthesis = NeedsParenthesis(rhs); + + lhs_id = verible::StringSpanOfSymbol(lhs); + const std::string fix = absl::StrCat( + lhs_id, " <= ", lhs_id, " ", + verible::StringSpanOfSymbol(operator_).substr(0, 1), " ", + needs_parenthesis ? "(" : "", verible::StringSpanOfSymbol(rhs), + needs_parenthesis ? ")" : "", ";"); autofixes.emplace_back( AutoFix("Substitute assignment operator for equivalent " @@ -149,8 +154,7 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, GetIncrementDecrementOperator(symbol); check_root = operand; - const absl::string_view identifier = - verible::StringSpanOfSymbol(*operand); + lhs_id = verible::StringSpanOfSymbol(*operand); // Extract just the operation. Just '+' from '++' const absl::string_view op = verible::StringSpanOfSymbol(*operator_).substr(0, 1); @@ -158,8 +162,8 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, // Equivalent nonblocking assignment // {'x++', '++x'} become 'x <= x + 1' // {'x--', '--x'} become 'x <= x - 1' - std::string fix = - absl::StrCat(identifier, " <= ", identifier, " ", op, " 1;"); + const std::string fix = + absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, " 1;"); autofixes.emplace_back( AutoFix("Substitute increment/decrement operator for " @@ -198,8 +202,12 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, } // Enqueue detected violation unless waived - if (!waived) { + if (waived) return; + + if (IsAutoFixSafe(symbol, lhs_id)) { violations_.insert(LintViolation(symbol, kMessage, context, autofixes)); + } else { + violations_.insert(LintViolation(symbol, kMessage, context)); } } // HandleSymbol() @@ -225,6 +233,7 @@ bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol, if (always_ff_matcher.Matches(symbol, &symbol_man)) { VLOG(4) << "always_ff @DEPTH=" << depth << std::endl; inside_ = depth; + CollectLocalReferences(symbol); } return false; } @@ -267,5 +276,94 @@ bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) { return false; } // LocalDeclaration() +bool AlwaysFFNonBlockingRule::NeedsParenthesis( + const verible::Symbol &rhs) const { + // Avoid inserting parenthesis for simple expressions + // For example: x &= 1 -> x <= x & 1, and not x <= x & (1) + // This could be more precise, but checking every specific + // case where parenthesis are needed is hard. Adding them + // doesn't hurt and the user can remove them if needed. + bool complex_rhs_expr = + verible::GetLeftmostLeaf(rhs) != verible::GetRightmostLeaf(rhs); + + // Check whether the rhs expression is already inside parenthesis + // Avoid searching for kParenGroups, this should be safe and faster + return complex_rhs_expr && + !absl::StrContains(verible::StringSpanOfSymbol(rhs), '('); +} + +void AlwaysFFNonBlockingRule::CollectLocalReferences( + const verible::Symbol &root) { + static const Matcher reference_matcher{NodekReference()}; + + const std::vector references_ = + verible::SearchSyntaxTree(root, reference_matcher); + + // Precompute the StringSpan of every identifier referenced to. + // Avoid recomputing it several times + references.resize(references_.size()); + std::transform(references_.cbegin(), references_.cend(), references.begin(), + [](const verible::TreeSearchMatch &match) { + return ReferenceWithId{ + match, verible::StringSpanOfSymbol(*match.match)}; + }); +} + +bool AlwaysFFNonBlockingRule::IsAutoFixSafe( + const verible::Symbol &faulting_assignment, + absl::string_view lhs_id) const { + std::vector::const_iterator itr = references.end(); + + // Let's assume that 'x' is the variable affected by the faulting + // assignment. In order to ensure that the autofix is safe we have + // to ensure that there is no later reference to 'x' + // + // Can't autofix Can autofix + // begin begin + // x = x + 1; x = x + 1; + // y = x; y <= y + 1; + // end end + // + // In practical terms: we'll scan the 'references' vector for kReferences + // to 'x' that appear after the faulting assignment in the always_ff block + + static const Matcher reference_matcher{NodekReference()}; + + // Extract kReferences inside the faulting expression + const std::vector references_ = + verible::SearchSyntaxTree(faulting_assignment, reference_matcher); + + // ref points to the latest reference to 'x' in our faulting expression + // 'x++', 'x = x + 1', 'x &= x + 1' + // ^ ^ ^ + // We need this to know from where to start searching for later references + // to 'x', and decide whether the AutoFix is safe or not + const verible::Symbol *ref = + std::find_if(std::rbegin(references_), std::rend(references_), + [&](const verible::TreeSearchMatch &m) { + return verible::StringSpanOfSymbol(*m.match) == lhs_id; + }) + ->match; + + // Shouldn't happen, sanity check to avoid crashes + if (!ref) return false; + + // Find the last reference to 'x' in the faulting assignment + itr = std::find_if( + std::begin(references), std::end(references), + [&](const ReferenceWithId &r) { return r.match.match == ref; }); + + // Search from the last reference to 'x' onwards. If there is any, + // we can't apply the autofix safely + itr = std::find_if( + std::next(itr), std::end(references), + [&](const ReferenceWithId &ref) { return ref.id == lhs_id; }); + + // Let's say 'x' is affected by a flagged operation { 'x = 1', 'x++', ... } + // We can safely autofix if after the flagged operation there are no + // more references to 'x' + return references.end() == itr; +} + } // namespace analysis } // namespace verilog diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.h b/verilog/analysis/checkers/always_ff_non_blocking_rule.h index e2d7bcd56d..05d4a78600 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.h +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule.h @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,12 +15,12 @@ #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_ -#include #include #include #include "common/analysis/lint_rule_status.h" #include "common/analysis/syntax_tree_lint_rule.h" +#include "common/analysis/syntax_tree_search.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" #include "verilog/analysis/descriptions.h" @@ -32,19 +32,29 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule { public: using rule_type = verible::SyntaxTreeLintRule; - static const LintRuleDescriptor& GetDescriptor(); + static const LintRuleDescriptor &GetDescriptor(); absl::Status Configure(absl::string_view configuration) final; - void HandleSymbol(const verible::Symbol& symbol, - const verible::SyntaxTreeContext& context) final; + void HandleSymbol(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context) final; verible::LintRuleStatus Report() const final; private: // Detects entering and leaving relevant code inside always_ff - bool InsideBlock(const verible::Symbol& symbol, int depth); + bool InsideBlock(const verible::Symbol &symbol, int depth); // Processes local declarations - bool LocalDeclaration(const verible::Symbol& symbol); + bool LocalDeclaration(const verible::Symbol &symbol); + // Check the need of parenthesis in certain autofixes + // Might have false positives. Example: + // Fixing 'x *= y + 1' requires adding parenthesis + // 'x <= x & (y + 1)' + bool NeedsParenthesis(const verible::Symbol &rhs) const; + // Collect local references inside an always_ff block + void CollectLocalReferences(const verible::Symbol &root); + // Check if is safe to autofix + bool IsAutoFixSafe(const verible::Symbol &faulting_assignment, + absl::string_view lhs_id) const; private: // Collected violations. @@ -69,6 +79,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule { // In-order stack of local variable names std::vector locals_; + + // NodekReference + string representation + // of the ID they're referring to + struct ReferenceWithId { + verible::TreeSearchMatch match; + absl::string_view id; + }; + // Collection of references to variables + // assumed to be in program order + std::vector references; }; } // namespace analysis diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc index 0b65864800..094e530da8 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc @@ -140,15 +140,11 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { TEST(AlwaysFFNonBlockingRule, AutoFixDefault) { const std::initializer_list kTestCases = { // Check we're not waiving local variables - {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", - "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, // Check we're ignoring modifying assignments {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, - {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", - "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, }; RunApplyFixCases(kTestCases, ""); @@ -158,8 +154,40 @@ TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) { const std::initializer_list kTestCases = { {"module m;\nalways_ff begin k &= 1; end\nendmodule", "module m;\nalways_ff begin k <= k & 1; end\nendmodule"}, + {"module m;\nalways_ff begin k &= a; end\nendmodule", + "module m;\nalways_ff begin k <= k & a; end\nendmodule"}, {"module m;\nalways_ff begin k |= (2 + 1); end\nendmodule", "module m;\nalways_ff begin k <= k | (2 + 1); end\nendmodule"}, + {"module m;\nalways_ff begin k *= 2 + 1; end\nendmodule", + "module m;\nalways_ff begin k <= k * (2 + 1); end\nendmodule"}, + {"module m;\nalways_ff begin a++; end\nendmodule", + "module m;\nalways_ff begin a <= a + 1; end\nendmodule"}, + {"module m;\nalways_ff begin ++a; end\nendmodule", + "module m;\nalways_ff begin a <= a + 1; end\nendmodule"}, + }; + + RunApplyFixCases( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixDontBreakCode) { + const std::initializer_list kTestCases = { + // We can't fix 'k &= 1', because it would affect + // 'p <= k' + {"module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a++;" + "end\nendmodule", + "module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a <= a + 1;" + "end\nendmodule"}, + // Correctly fix despide there is a reference to 'k' in the rhs + {"module m;\nalways_ff begin k = k + 1; end\nendmodule", + "module m;\nalways_ff begin k <= k + 1; end\nendmodule"}, + // TODO: Add more tests }; RunApplyFixCases(