Address third row of comments:

- Perform check before doing SymbolCastToNode - Call function with broken input (leafs, ...) - Add comments, style cleanup.
mfkiwl · Apr 4, 2023 · ab991cf · ab991cf
1 parent 820e8f8
commit ab991cf
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 14 deletions.
diff --git a/verilog/CST/expression.cc b/verilog/CST/expression.cc
@@ -100,7 +100,9 @@ const verible::Symbol* GetConditionExpressionFalseCase(
 
 const verible::TokenInfo* GetUnaryPrefixOperator(
     const verible::Symbol& symbol) {
-  const SyntaxTreeNode* node = &verible::SymbolCastToNode(symbol);
+  const SyntaxTreeNode* node = symbol.Kind() == SymbolKind::kNode
+                                   ? &verible::SymbolCastToNode(symbol)
+                                   : nullptr;
   if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) {
     return nullptr;
   }
@@ -110,7 +112,9 @@ const verible::TokenInfo* GetUnaryPrefixOperator(
 }
 
 const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol& symbol) {
-  const SyntaxTreeNode* node = &verible::SymbolCastToNode(symbol);
+  const SyntaxTreeNode* node = symbol.Kind() == SymbolKind::kNode
+                                   ? &verible::SymbolCastToNode(symbol)
+                                   : nullptr;
   if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) {
     return nullptr;
   }

diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.cc
@@ -446,6 +446,7 @@ TEST(GetUnaryPrefixOperator, Exprs) {
     } else {
       EXPECT_NE(NodeEnum(last_node->Tag().tag),
                 NodeEnum::kUnaryPrefixExpression);
+      EXPECT_FALSE(GetUnaryPrefixOperand(*last_node));
     }
   }
 }
@@ -472,6 +473,7 @@ TEST(GetUnaryPrefixOperand, Exprs) {
     } else {
       EXPECT_NE(NodeEnum(last_node->Tag().tag),
                 NodeEnum::kUnaryPrefixExpression);
+      EXPECT_FALSE(GetUnaryPrefixOperand(*last_node));
     }
   }
 }

diff --git a/verilog/analysis/checkers/forbid_negative_array_dim.cc b/verilog/analysis/checkers/forbid_negative_array_dim.cc
@@ -61,27 +61,30 @@ static const Matcher& UnaryPrefixExprMatcher() {
 
 void ForbidNegativeArrayDim::HandleSymbol(
     const verible::Symbol& symbol, const verible::SyntaxTreeContext& context) {
+  // This only works for simple unary expressions. They can't be nested inside
+  // other expressions. This avoids false positives of the form:
+  // logic l [10+(-5):0], logic l[-(-5):0]
   if (!context.IsInsideFirst(
           {NodeEnum::kPackedDimensions, NodeEnum::kUnpackedDimensions},
           {NodeEnum::kBinaryExpression, NodeEnum::kUnaryPrefixExpression})) {
     return;
   }
 
   verible::matcher::BoundSymbolManager manager;
-  if (UnaryPrefixExprMatcher().Matches(symbol, &manager)) {
-    int value = 0;
+  if (!UnaryPrefixExprMatcher().Matches(symbol, &manager)) return;
 
-    // Extract operand and operator from kUnaryPrefixExpression
-    const verible::TokenInfo* u_operator =
-        verilog::GetUnaryPrefixOperator(symbol);
-    const verible::Symbol* operand = verilog::GetUnaryPrefixOperand(symbol);
+  // As we've previously ensured that this symbol is a kUnaryPrefixExpression
+  // both its operator and operand are defined
+  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 && value > 0 && u_operator->text() == "-") {
-      const verible::TokenInfo token(TK_OTHER,
-                                     verible::StringSpanOfSymbol(symbol));
-      violations_.insert(verible::LintViolation(token, kMessage, context));
-    }
+  int value = 0;
+  const bool is_constant = verilog::ConstantIntegerValue(*operand, &value);
+  if (is_constant && value > 0 && u_operator->text() == "-") {
+    const verible::TokenInfo token(TK_OTHER,
+                                   verible::StringSpanOfSymbol(symbol));
+    violations_.insert(verible::LintViolation(token, kMessage, context));
   }
 }