Address 2nd review

- Change auto for the actual types - Add tests for new functions for unary expressions - Add checks for correct input type for new functions. Return nullptr when appropiate - Remove typo when checking boolean value (redundant > 0)
chipsalliance · Apr 1, 2023 · 820e8f8 · 820e8f8
1 parent b8e4591
commit 820e8f8
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 8 deletions.
diff --git a/verilog/CST/expression.cc b/verilog/CST/expression.cc
@@ -100,15 +100,21 @@ const verible::Symbol* GetConditionExpressionFalseCase(
 
 const verible::TokenInfo* GetUnaryPrefixOperator(
     const verible::Symbol& symbol) {
-  const auto& children = verible::SymbolCastToNode(symbol).children();
-  const auto leaf_symbol = children.front().get();
+  const SyntaxTreeNode* node = &verible::SymbolCastToNode(symbol);
+  if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) {
+    return nullptr;
+  }
+  const verible::Symbol* leaf_symbol = node->children().front().get();
   return &verible::down_cast<const verible::SyntaxTreeLeaf*>(leaf_symbol)
               ->get();
 }
 
 const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol& symbol) {
-  const auto& children = verible::SymbolCastToNode(symbol).children();
-  return children.back().get();
+  const SyntaxTreeNode* node = &verible::SymbolCastToNode(symbol);
+  if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) {
+    return nullptr;
+  }
+  return node->children().back().get();
 }
 
 std::vector<verible::TreeSearchMatch> FindAllBinaryOperations(

diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.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.
@@ -423,6 +423,59 @@ TEST(GetConditionExpressionFalseCaseTest, Various) {
   }
 }
 
+TEST(GetUnaryPrefixOperator, Exprs) {
+  const std::pair<const char*, const char*> kTestCases[] = {
+      {"-(2)", "-"},    {"-1", "-"},        {"&1", "&"},
+      {"666", nullptr}, {"1 + 2", nullptr}, {"!1", "!"},
+  };
+  for (auto test : kTestCases) {
+    const auto analyzer_ptr =
+        AnalyzeVerilogExpression(test.first, "<file>", kDefaultPreprocess);
+    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);
+
+    if (test.second) {
+      const verible::SyntaxTreeNode& unary_expr =
+          verible::SymbolCastToNode(*last_node);
+      EXPECT_EQ(NodeEnum(unary_expr.Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+      EXPECT_EQ(test.second, GetUnaryPrefixOperator(unary_expr)->text());
+    } else {
+      EXPECT_NE(NodeEnum(last_node->Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+    }
+  }
+}
+
+TEST(GetUnaryPrefixOperand, Exprs) {
+  const std::pair<const char*, const char*> kTestCases[] = {
+      {"-1", ""}, {"&1", ""}, {"666", nullptr}, {"1 + 2", nullptr}, {"!1", ""},
+  };
+  for (auto test : kTestCases) {
+    const auto analyzer_ptr =
+        AnalyzeVerilogExpression(test.first, "<file>", kDefaultPreprocess);
+    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);
+
+    if (test.second) {
+      const verible::SyntaxTreeNode& unary_expr =
+          verible::SymbolCastToNode(*last_node);
+      EXPECT_EQ(NodeEnum(unary_expr.Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+      EXPECT_TRUE(GetUnaryPrefixOperand(unary_expr));
+    } else {
+      EXPECT_NE(NodeEnum(last_node->Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+    }
+  }
+}
+
 TEST(FindAllConditionExpressionsTest, Various) {
   constexpr int kTag = 1;  // value doesn't matter
   const SyntaxTreeSearchTestCase kTestCases[] = {

diff --git a/verilog/analysis/checkers/forbid_negative_array_dim.cc b/verilog/analysis/checkers/forbid_negative_array_dim.cc
@@ -72,11 +72,12 @@ void ForbidNegativeArrayDim::HandleSymbol(
     int value = 0;
 
     // Extract operand and operator from kUnaryPrefixExpression
-    const auto u_operator = verilog::GetUnaryPrefixOperator(symbol);
-    const auto operand = verilog::GetUnaryPrefixOperand(symbol);
+    const verible::TokenInfo* u_operator =
+        verilog::GetUnaryPrefixOperator(symbol);
+    const verible::Symbol* operand = verilog::GetUnaryPrefixOperand(symbol);
 
     const bool is_constant = verilog::ConstantIntegerValue(*operand, &value);
-    if (is_constant > 0 && value > 0 && u_operator->text() == "-") {
+    if (is_constant && value > 0 && u_operator->text() == "-") {
       const verible::TokenInfo token(TK_OTHER,
                                      verible::StringSpanOfSymbol(symbol));
       violations_.insert(verible::LintViolation(token, kMessage, context));