From ccd529d693f458079fe58cbbb025c7e2024b1869 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 5 Apr 2024 23:16:59 +1100 Subject: [PATCH 01/14] Pull request https://github.com/chipsalliance/verible/pull/1336 seems to have stalled. Copied the code from https://github.com/suzizecat/verible/commit/0b087d72cfa58e9e8c34a164d4abaeda9880ca7d and rebased it to the head. Updated the tests and formatted. --- verilog/analysis/checkers/BUILD | 32 ++++ .../analysis/checkers/explicit_begin_rule.cc | 159 ++++++++++++++++++ .../analysis/checkers/explicit_begin_rule.h | 67 ++++++++ .../checkers/explicit_begin_rule_test.cc | 93 ++++++++++ 4 files changed, 351 insertions(+) create mode 100644 verilog/analysis/checkers/explicit_begin_rule.cc create mode 100644 verilog/analysis/checkers/explicit_begin_rule.h create mode 100644 verilog/analysis/checkers/explicit_begin_rule_test.cc diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index dc1177397..5ded24a4b 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -24,6 +24,7 @@ cc_library( ":disable-statement-rule", ":endif-comment-rule", ":enum-name-style-rule", + ":explicit-begin-rule", ":explicit-function-lifetime-rule", ":explicit-function-task-parameter-type-rule", ":explicit-parameter-storage-type-rule", @@ -1249,6 +1250,37 @@ cc_test( ], ) +cc_library( + name = "explicit-begin-rule", + srcs = ["explicit_begin_rule.cc"], + hdrs = ["explicit_begin_rule.h"], + deps = [ + "//common/analysis:lint-rule-status", + "//common/analysis:token-stream-lint-rule", + "//common/strings:comment-utils", + "//common/text:token-info", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/strings", + ], + alwayslink = 1, +) + +cc_test( + name = "explicit-begin-rule_test", + srcs = ["explicit_begin_rule_test.cc"], + deps = [ + ":explicit-begin-rule", + "//common/analysis:linter-test-utils", + "//common/analysis:token-stream-linter-test-utils", + "//common/text:symbol", + "//verilog/analysis:verilog-analyzer", + "//verilog/parser:verilog-token-enum", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "explicit-function-lifetime-rule", srcs = ["explicit_function_lifetime_rule.cc"], diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc new file mode 100644 index 000000000..bf1dde2d4 --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -0,0 +1,159 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/explicit_begin_rule.h" + +#include +#include +#include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/strings/comment_utils.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { + +using verible::AutoFix; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::TokenInfo; +using verible::TokenStreamLintRule; + +// Register the lint rule +VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); + +static const char kMessage[] = + "All block construct shall explicitely use begin/end."; + +const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "explicit-begin", + .topic = "explicit-begin", + .desc = + "Checks that a Verilog ``begin`` directive follows all " + "if, else and for loops.", + }; + return d; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Responds to a token by updating the state of the analysis. + bool raise_violation = false; + switch (state_) { + case State::kNormal: { + // On if/else/for tokens; + // Also, skip all conditional statement after a for or a if + // Then, expect a `begin` token or record a violation. + switch (token.token_enum()) { + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_else: + last_condition_start_ = token; + end_of_condition_statement_ = token; + state_ = State::kInElse; + break; + default: + break; + } // switch (token) + break; + } + case State::kInElse: { + // If we are in a else statement, we can either have a if (and need to + // skip cond. statement) Or directly wait for a begin. We make use of the + // boolean raise_violation in order to avoid code duplication. + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + case TK_NEWLINE: + break; + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_begin: + state_ = State::kNormal; + break; + default: + raise_violation = true; + break; + } // switch (token) + break; + } + case State::kInCondition: { + if (token.text() == "(") { + condition_expr_level_++; + } else if (token.text() == ")") { + condition_expr_level_--; + if (condition_expr_level_ == 0) { + end_of_condition_statement_ = token; + state_ = State::kExpectBegin; + } + } + break; + } + case State::kExpectBegin: { + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + case TK_NEWLINE: + break; + case TK_begin: + // If we got our begin token, we go back to normal status + state_ = State::kNormal; + break; + default: { + raise_violation = true; + break; + } + } // switch (token) + break; + } + } // switch (state_) + + if (raise_violation) { + violations_.insert(LintViolation( + last_condition_start_, + absl::StrCat(kMessage, " Expected begin, got ", token.text()))); + + // Once the violation is raised, we go back to a normal, default, state + condition_expr_level_ = 0; + state_ = State::kNormal; + raise_violation = false; + } +} + +LintRuleStatus ExplicitBeginRule::Report() const { + return LintRuleStatus(violations_, GetDescriptor()); +} + +} // namespace analysis +} // namespace verilog \ No newline at end of file diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h new file mode 100644 index 000000000..e90554c52 --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -0,0 +1,67 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ + +#include +#include +#include + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// Detects whether if and for loop statements use verilog block statements (begin/end) +class ExplicitBeginRule : public verible::TokenStreamLintRule { + public: + using rule_type = verible::TokenStreamLintRule; + + static const LintRuleDescriptor &GetDescriptor(); + + ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} + + void HandleToken(const verible::TokenInfo &token) final; + + verible::LintRuleStatus Report() const final; + + private: + // States of the internal token-based analysis. + enum class State { kNormal, kInCondition, kInElse, kExpectBegin }; + + // Internal lexical analysis state. + State state_; + + // Level of nested parenthesis when analizing conditional expressions. + int condition_expr_level_; + + // Token information for the last seen block opening (for/if/else). + verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); + verible::TokenInfo end_of_condition_statement_ = + verible::TokenInfo::EOFToken(); + + // Collection of found violations. + std::set violations_; + + void trigger_violation_(); +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_ENDIF_COMMENT_RULE_H_ \ No newline at end of file diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc new file mode 100644 index 000000000..317ed863a --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -0,0 +1,93 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/explicit_begin_rule.h" + +#include + +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/token_stream_linter_test_utils.h" +#include "common/text/symbol.h" +#include "gtest/gtest.h" +#include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunApplyFixCases; +using verible::RunLintTestCases; + +// Tests that space-only text passes. +TEST(ExplicitBeginRuleTest, AcceptsBlank) { + const std::initializer_list kTestCases = { + {""}, + {" "}, + {"\n"}, + {" \n\n"}, + }; + RunLintTestCases(kTestCases); +} + +// Tests that properly matched if/begin passes. +TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { + const std::initializer_list kTestCases = { + {"if (FOO) begin"}, + {"if (FOO)\n begin"}, + {"if (FOO) //Comment\n begin"}, + {"else begin \n FOO"}, + {"else \nbegin \n FOO"}, + {"else //Comment\n begin \n FOO"}, + {"else \n //Comment\n begin \n FOO"}, + {"else if (FOO) begin"}, + {"else if (FOO)\n begin"}, + {"else if (FOO) //Comment\n begin"}, + {"else if (FOO)\n //Comment\n begin"}, + {"for(i = 0; i < N; i++) begin"}, + {"for(i = 0; i < N; i++)\nbegin"}, + {"for(i = 0; i < N; i++) // Comment\n begin"}, + {"for(i = 0; i < N; i++)\n // Comment\nbegin"}, + // {"forever begin"}, // Not supported + }; + RunLintTestCases(kTestCases); +} + +// Tests that unmatched block/begin fails is detected. +TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {{TK_if, "if"}, " (FOO)\n BAR"}, + {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_else, "else"}, " \n \n FOO"}, + {{TK_else, "else"}, " //Comment\n FOO"}, + {{TK_else, "else"}, " \n //Comment\n FOO"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, + {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, + // {{TK_forever, "forever"}, "a <= 1'b1;"}, // Not supported + }; + RunLintTestCases(kTestCases); +} + +} // namespace +} // namespace analysis +} // namespace verilog \ No newline at end of file From cbced8ab5829b05a1e3531cf2a7accfe8e573401 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 12 Apr 2024 23:40:09 +1000 Subject: [PATCH 02/14] Added more common constructs that should have begin-end block statements. --- .../analysis/checkers/explicit_begin_rule.cc | 123 ++++++++++++------ .../analysis/checkers/explicit_begin_rule.h | 6 +- .../checkers/explicit_begin_rule_test.cc | 120 +++++++++++++++-- 3 files changed, 194 insertions(+), 55 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index bf1dde2d4..80ad13460 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -42,7 +42,7 @@ using verible::TokenStreamLintRule; VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); static const char kMessage[] = - "All block construct shall explicitely use begin/end."; + " block constructs shall explicitly use begin/end."; const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -50,51 +50,93 @@ const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { .topic = "explicit-begin", .desc = "Checks that a Verilog ``begin`` directive follows all " - "if, else and for loops.", + "if, else, always, always_comb, always_latch, always_ff," + " forever, initial, for, foreach and while statements.", }; return d; } void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Ignore all white space and comments and return immediately + switch (token.token_enum()) { + case TK_SPACE: + case TK_NEWLINE: + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + return; + default: + break; + } + // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { - case State::kNormal: { - // On if/else/for tokens; - // Also, skip all conditional statement after a for or a if - // Then, expect a `begin` token or record a violation. + case State::kNormal: { switch (token.token_enum()) { - case TK_if: + // After token expect "begin" + case TK_always_comb: + case TK_always_latch: + case TK_forever: + case TK_initial: + start_token_ = token; + state_ = State::kExpectBegin; + break; + // After token expect a "condition" followed by "begin". NOTE: there may + // be tokens prior to the condition (like in an "always_ff" statement) + // and these are all ignored. + case TK_always_ff: + case TK_foreach: case TK_for: + case TK_if: + case TK_while: condition_expr_level_ = 0; - last_condition_start_ = token; + start_token_ = token; state_ = State::kInCondition; break; + // always gets special handling, as somtimes there is a "condition" or + // not before a "begin". + case TK_always: + condition_expr_level_ = 0; + start_token_ = token; + state_ = State::kInAlways; + break; + // else is also special as "if" or "begin" can follow case TK_else: - last_condition_start_ = token; - end_of_condition_statement_ = token; + start_token_ = token; state_ = State::kInElse; break; default: break; - } // switch (token) + } break; } - case State::kInElse: { - // If we are in a else statement, we can either have a if (and need to - // skip cond. statement) Or directly wait for a begin. We make use of the - // boolean raise_violation in order to avoid code duplication. + case State::kInAlways: { + // always is a little more complicated in that it can be imediattly + // followed by a "begin" or followed by some special characters ("@" or + // "*") and maybe a condition. switch (token.token_enum()) { - case TK_SPACE: // stay in the same state + case '@': + case '*': + break; + case TK_begin: + state_ = State::kNormal; break; - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - case TK_NEWLINE: + case '(': + condition_expr_level_ = 1; + state_ = State::kInCondition; + break; + default: + raise_violation = true; break; + } + break; + } + case State::kInElse: { + // An else statement can be followed by either a begin or an if. + switch (token.token_enum()) { case TK_if: - case TK_for: condition_expr_level_ = 0; - last_condition_start_ = token; + start_token_ = token; state_ = State::kInCondition; break; case TK_begin: @@ -103,29 +145,34 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { default: raise_violation = true; break; - } // switch (token) + } break; } case State::kInCondition: { - if (token.text() == "(") { - condition_expr_level_++; - } else if (token.text() == ")") { - condition_expr_level_--; - if (condition_expr_level_ == 0) { - end_of_condition_statement_ = token; - state_ = State::kExpectBegin; + // The last token expects a condition statement enclosed in a pair of + // parentheses "()". This process also ignores any tokens between the last + // token and the opening parentheses which simplifies "always_ff". + switch (token.token_enum()) { + case '(': { + condition_expr_level_++; + break; + } + case ')': { + condition_expr_level_--; + if (condition_expr_level_ == 0) { + state_ = State::kExpectBegin; + } + } + default: { + // throw away everything else + break; } } break; } case State::kExpectBegin: { + // The next token must be a "begin" switch (token.token_enum()) { - case TK_SPACE: // stay in the same state - break; - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - case TK_NEWLINE: - break; case TK_begin: // If we got our begin token, we go back to normal status state_ = State::kNormal; @@ -134,15 +181,15 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { raise_violation = true; break; } - } // switch (token) + } break; } } // switch (state_) if (raise_violation) { violations_.insert(LintViolation( - last_condition_start_, - absl::StrCat(kMessage, " Expected begin, got ", token.text()))); + start_token_, + absl::StrCat(start_token_.text(), kMessage, " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state condition_expr_level_ = 0; diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index e90554c52..187452382 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -42,7 +42,7 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { private: // States of the internal token-based analysis. - enum class State { kNormal, kInCondition, kInElse, kExpectBegin }; + enum class State { kNormal, kInAlways, kInCondition, kInElse, kExpectBegin }; // Internal lexical analysis state. State state_; @@ -51,9 +51,7 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { int condition_expr_level_; // Token information for the last seen block opening (for/if/else). - verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); - verible::TokenInfo end_of_condition_statement_ = - verible::TokenInfo::EOFToken(); + verible::TokenInfo start_token_ = verible::TokenInfo::EOFToken(); // Collection of found violations. std::set violations_; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 317ed863a..8ff18d0a3 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -45,22 +45,70 @@ TEST(ExplicitBeginRuleTest, AcceptsBlank) { // Tests that properly matched if/begin passes. TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { const std::initializer_list kTestCases = { - {"if (FOO) begin"}, - {"if (FOO)\n begin"}, - {"if (FOO) //Comment\n begin"}, + {"if (FOO) /*block comment */ begin a <= 1;"}, + {"if (FOO) begin a <= 1;"}, + {"if (FOO)begin : name_statement a <= 1;"}, + {"if (FOO)\n begin a <= 1;"}, + {"if (FOO) //Comment\n begin a <= 1;"}, + {"else begin \n FOO"}, {"else \nbegin \n FOO"}, {"else //Comment\n begin \n FOO"}, {"else \n //Comment\n begin \n FOO"}, - {"else if (FOO) begin"}, - {"else if (FOO)\n begin"}, - {"else if (FOO) //Comment\n begin"}, - {"else if (FOO)\n //Comment\n begin"}, - {"for(i = 0; i < N; i++) begin"}, - {"for(i = 0; i < N; i++)\nbegin"}, - {"for(i = 0; i < N; i++) // Comment\n begin"}, - {"for(i = 0; i < N; i++)\n // Comment\nbegin"}, - // {"forever begin"}, // Not supported + {"else if (FOO) begin a <= 1;"}, + {"else if (FOO)\n begin a <= 1;"}, + {"else if (FOO) //Comment\n begin a <= 1;"}, + {"else if (FOO)\n //Comment\n begin a <= 1;"}, + + {"for(i = 0; i < N; i++) begin a <= 1;"}, + {"for(i = 0; i < N; i++)\nbegin a <= 1;"}, + {"for(i = 0; i < N; i++) // Comment\n begin a <= 1;"}, + {"for(i = 0; i < N; i++)\n // Comment\nbegin a <= 1;"}, + + {"foreach(array[i]) begin a <= 1;"}, + {"foreach(array[i])\nbegin a <= 1;"}, + {"foreach(array[i]) // Comment\n begin a <= 1;"}, + {"foreach(array[i])\n // Comment\nbegin a <= 1;"}, + + {"while (a < 3) begin a = a + 1;"}, + {"while(a < 3)\nbegin a = a + 1;"}, + {"while (a < 3) // Comment\n begin a = a + 1;"}, + {"while(a < 3)\n // Comment\nbegin a = a + 1;"}, + + {"forever begin a <= 1;"}, + {"forever\nbegin a <= 1;"}, + {"forever // Comment\n begin a <= 1;"}, + {"forever\n // Comment\nbegin a <= 1;"}, + + {"initial begin a <= 1;"}, + {"initial\nbegin a <= 1;"}, + {"initial // Comment\n begin a <= 1;"}, + {"initial\n // Comment\nbegin a <= 1;"}, + + {"always_comb begin a = 1;"}, + {"always_comb\nbegin a = 1;"}, + {"always_comb // Comment\n begin a = 1;"}, + {"always_comb\n // Comment\nbegin a = 1;"}, + + {"always_latch begin a <= 1;"}, + {"always_latch\nbegin a <= 1;"}, + {"always_latch // Comment\n begin a <= 1;"}, + {"always_latch\n // Comment\nbegin a <= 1;"}, + + {"always_ff @( a or b) begin a <= 1;"}, + {"always_ff @ ( a or b)\nbegin a <= 1;"}, + {"always_ff @( (a) and b) // Comment\n begin a <= 1;"}, + {"always_ff @( a or ((b)))\n // Comment\nbegin a <= 1;"}, + + {"always @( a or b) begin a <= 1;"}, + {"always @ ( a or b)\nbegin a <= 1;"}, + {"always @( (a) and b) // Comment\n begin a <= 1;"}, + {"always @( a or ((b)))\n // Comment\nbegin a <= 1;"}, + {"always@* begin a = 1'b1;"}, + {"always@(*) begin a = 1'b1;"}, + {"always @* begin a = 1'b1;"}, + {"always begin a = 1'b1;"}, + {"always begin #10 a = 1'b1;"}, }; RunLintTestCases(kTestCases); } @@ -71,6 +119,7 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_if, "if"}, " (FOO) BAR"}, {{TK_if, "if"}, " (FOO)\n BAR"}, {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {{TK_else, "else"}, " \n FOO"}, {{TK_else, "else"}, " \n \n FOO"}, {{TK_else, "else"}, " //Comment\n FOO"}, @@ -79,11 +128,56 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, - // {{TK_forever, "forever"}, "a <= 1'b1;"}, // Not supported + + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i])\n // Comment\n a <= 1'b1;"}, + + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, + {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, + + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_forever, "forever"}, "\n a <= 1'b1;"}, + {{TK_forever, "forever"}, " // Comment \n a <= 1'b1;"}, + {{TK_forever, "forever"}, "\n // Comment\n a <= 1'b1;"}, + + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_initial, "initial"}, "\n a = 1'b1;"}, + {{TK_initial, "initial"}, " // Comment \n a = 1'b1;"}, + {{TK_initial, "initial"}, "\n // Comment\n a = 1'b1;"}, + + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, "\n a = 1'b1;"}, + {{TK_always_comb, "always_comb"}, " // Comment \n a = 1'b1;"}, + {{TK_always_comb, "always_comb"}, "\n // Comment\n a = 1'b1;"}, + + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, "\n a = 1'b1;"}, + {{TK_always_latch, "always_latch"}, " // Comment \n a = 1'b1;"}, + {{TK_always_latch, "always_latch"}, "\n // Comment\n a = 1'b1;"}, + + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, "@(a or b)\n a <= 1'b1;"}, + {{TK_always_ff, "always_ff"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always_ff, "always_ff"}, "@(a || b)\n // Comment\n a <= 1'b1;"}, + + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + {{TK_always, "always"}, "@(a or b)\n a = 1'b1;"}, + {{TK_always, "always"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always, "always"}, "@(a || b)\n // Comment\n <= 1'b1;"}, + {{TK_always, "always"}, "@* a = 1'b1;"}, + {{TK_always, "always"}, "@(*) a = 1'b1;"}, + {{TK_always, "always"}, " @* a = 1'b1;"}, + {{TK_always, "always"}, " a = 1'b1;"}, + {{TK_always, "always"}, " #10 a = 1'b1;"}, }; RunLintTestCases(kTestCases); } From 576dd700dbf9edcbe53023a81d47da6d682d56f3 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sun, 14 Apr 2024 00:10:44 +1000 Subject: [PATCH 03/14] Added rule configuration, allowing users to disable explicit begin end blocks for each supported statement. --- verilog/analysis/checkers/BUILD | 1 + .../analysis/checkers/explicit_begin_rule.cc | 103 ++++++- .../analysis/checkers/explicit_begin_rule.h | 28 +- .../checkers/explicit_begin_rule_test.cc | 268 ++++++++++++++++-- 4 files changed, 373 insertions(+), 27 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 5ded24a4b..6d9293d5c 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1258,6 +1258,7 @@ cc_library( "//common/analysis:lint-rule-status", "//common/analysis:token-stream-lint-rule", "//common/strings:comment-utils", + "//common/text:config-utils", "//common/text:token-info", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 80ad13460..fbf401688 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -24,6 +24,7 @@ #include "common/analysis/lint_rule_status.h" #include "common/analysis/token_stream_lint_rule.h" #include "common/strings/comment_utils.h" +#include "common/text/config_utils.h" #include "common/text/token_info.h" #include "verilog/analysis/descriptions.h" #include "verilog/analysis/lint_rule_registry.h" @@ -52,10 +53,91 @@ const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { "Checks that a Verilog ``begin`` directive follows all " "if, else, always, always_comb, always_latch, always_ff," " forever, initial, for, foreach and while statements.", + .param = + { + {"if_enable", "true", + "All if statements require an explicit begin-end block"}, + {"else_enable", "true", + "All else statements require an explicit begin-end block"}, + {"always_enable", "true", + "All always statements require an explicit begin-end block"}, + {"always_comb_enable", "true", + "All always_comb statements require an explicit begin-end " + "block"}, + {"always_latch_enable", "true", + "All always_latch statements require an explicit begin-end " + "block"}, + {"always_ff_enable", "true", + "All always_ff statements require an explicit begin-end block"}, + {"forever_enable", "true", + "All forever statements require an explicit begin-end block"}, + {"initial_enable", "true", + "All initial statements require an explicit begin-end block"}, + {"for_enable", "true", + "All for statements require an explicit begin-end block"}, + {"foreach_enable", "true", + "All foreach statements require an explicit begin-end block"}, + {"while_enable", "true", + "All while statements require an explicit begin-end block"}, + }, }; return d; } +absl::Status ExplicitBeginRule::Configure(absl::string_view configuration) { + static const std::vector supported_statements = { + "if", "else", "always", "always_comb", + "always_latch", "always_ff", "forever", "initial", + "for", "foreach", "while"}; // same sequence as enum + // StyleChoicesBits + + using verible::config::SetBool; + return verible::ParseNameValues( + configuration, + { + {"if_enable", SetBool(&if_enable_)}, + {"else_enable", SetBool(&else_enable_)}, + {"always_enable", SetBool(&always_enable_)}, + {"always_comb_enable", SetBool(&always_comb_enable_)}, + {"always_latch_enable", SetBool(&always_latch_enable_)}, + {"always_ff_enable", SetBool(&always_ff_enable_)}, + {"forever_enable", SetBool(&forever_enable_)}, + {"initial_enable", SetBool(&initial_enable_)}, + {"for_enable", SetBool(&for_enable_)}, + {"foreach_enable", SetBool(&foreach_enable_)}, + {"while_enable", SetBool(&while_enable_)}, + }); +} + +bool ExplicitBeginRule::IsTokenEnabled(const TokenInfo &token) { + switch (token.token_enum()) { + case TK_always_comb: + return always_comb_enable_; + case TK_always_latch: + return always_latch_enable_; + case TK_forever: + return forever_enable_; + case TK_initial: + return initial_enable_; + case TK_always_ff: + return always_ff_enable_; + case TK_foreach: + return foreach_enable_; + case TK_for: + return for_enable_; + case TK_if: + return if_enable_; + case TK_while: + return while_enable_; + case TK_always: + return always_enable_; + case TK_else: + return else_enable_; + default: + return false; + } +} + void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // Ignore all white space and comments and return immediately switch (token.token_enum()) { @@ -71,7 +153,11 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { - case State::kNormal: { + case State::kNormal: { + if (!IsTokenEnabled(token)) { + return; + } + switch (token.token_enum()) { // After token expect "begin" case TK_always_comb: @@ -106,6 +192,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { state_ = State::kInElse; break; default: + // Do nothing break; } break; @@ -135,9 +222,13 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // An else statement can be followed by either a begin or an if. switch (token.token_enum()) { case TK_if: - condition_expr_level_ = 0; - start_token_ = token; - state_ = State::kInCondition; + if (if_enable_) { + condition_expr_level_ = 0; + start_token_ = token; + state_ = State::kInCondition; + } else { + state_ = State::kNormal; + } break; case TK_begin: state_ = State::kNormal; @@ -188,8 +279,8 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { if (raise_violation) { violations_.insert(LintViolation( - start_token_, - absl::StrCat(start_token_.text(), kMessage, " Expected begin, got ", token.text()))); + start_token_, absl::StrCat(start_token_.text(), kMessage, + " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state condition_expr_level_ = 0; diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 187452382..2117a1b24 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -27,13 +27,18 @@ namespace verilog { namespace analysis { -// Detects whether if and for loop statements use verilog block statements (begin/end) +using verible::TokenInfo; + +// Detects whether if and for loop statements use verilog block statements +// (begin/end) class ExplicitBeginRule : public verible::TokenStreamLintRule { public: using rule_type = verible::TokenStreamLintRule; static const LintRuleDescriptor &GetDescriptor(); + absl::Status Configure(absl::string_view configuration) final; + ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} void HandleToken(const verible::TokenInfo &token) final; @@ -41,22 +46,35 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { verible::LintRuleStatus Report() const final; private: + bool IsTokenEnabled(const TokenInfo &token); + // States of the internal token-based analysis. enum class State { kNormal, kInAlways, kInCondition, kInElse, kExpectBegin }; // Internal lexical analysis state. State state_; - // Level of nested parenthesis when analizing conditional expressions. + // Level of nested parenthesis when analysing conditional expressions. int condition_expr_level_; - // Token information for the last seen block opening (for/if/else). + // Configuration + bool if_enable_ = true; + bool else_enable_ = true; + bool always_enable_ = true; + bool always_comb_enable_ = true; + bool always_latch_enable_ = true; + bool always_ff_enable_ = true; + bool forever_enable_ = true; + bool initial_enable_ = true; + bool for_enable_ = true; + bool foreach_enable_ = true; + bool while_enable_ = true; + + // Token that requires blocking statement. verible::TokenInfo start_token_ = verible::TokenInfo::EOFToken(); // Collection of found violations. std::set violations_; - - void trigger_violation_(); }; } // namespace analysis diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 8ff18d0a3..6c3bc5adb 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -29,6 +29,7 @@ namespace { using verible::LintTestCase; using verible::RunApplyFixCases; +using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; // Tests that space-only text passes. @@ -50,7 +51,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"if (FOO)begin : name_statement a <= 1;"}, {"if (FOO)\n begin a <= 1;"}, {"if (FOO) //Comment\n begin a <= 1;"}, - + {"else begin \n FOO"}, {"else \nbegin \n FOO"}, {"else //Comment\n begin \n FOO"}, @@ -59,12 +60,12 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"else if (FOO)\n begin a <= 1;"}, {"else if (FOO) //Comment\n begin a <= 1;"}, {"else if (FOO)\n //Comment\n begin a <= 1;"}, - + {"for(i = 0; i < N; i++) begin a <= 1;"}, {"for(i = 0; i < N; i++)\nbegin a <= 1;"}, {"for(i = 0; i < N; i++) // Comment\n begin a <= 1;"}, {"for(i = 0; i < N; i++)\n // Comment\nbegin a <= 1;"}, - + {"foreach(array[i]) begin a <= 1;"}, {"foreach(array[i])\nbegin a <= 1;"}, {"foreach(array[i]) // Comment\n begin a <= 1;"}, @@ -74,7 +75,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"while(a < 3)\nbegin a = a + 1;"}, {"while (a < 3) // Comment\n begin a = a + 1;"}, {"while(a < 3)\n // Comment\nbegin a = a + 1;"}, - + {"forever begin a <= 1;"}, {"forever\nbegin a <= 1;"}, {"forever // Comment\n begin a <= 1;"}, @@ -84,7 +85,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"initial\nbegin a <= 1;"}, {"initial // Comment\n begin a <= 1;"}, {"initial\n // Comment\nbegin a <= 1;"}, - + {"always_comb begin a = 1;"}, {"always_comb\nbegin a = 1;"}, {"always_comb // Comment\n begin a = 1;"}, @@ -94,7 +95,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always_latch\nbegin a <= 1;"}, {"always_latch // Comment\n begin a <= 1;"}, {"always_latch\n // Comment\nbegin a <= 1;"}, - + {"always_ff @( a or b) begin a <= 1;"}, {"always_ff @ ( a or b)\nbegin a <= 1;"}, {"always_ff @( (a) and b) // Comment\n begin a <= 1;"}, @@ -110,6 +111,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always begin a = 1'b1;"}, {"always begin #10 a = 1'b1;"}, }; + RunLintTestCases(kTestCases); } @@ -119,7 +121,7 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_if, "if"}, " (FOO) BAR"}, {{TK_if, "if"}, " (FOO)\n BAR"}, {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, - + {{TK_else, "else"}, " \n FOO"}, {{TK_else, "else"}, " \n \n FOO"}, {{TK_else, "else"}, " //Comment\n FOO"}, @@ -128,12 +130,12 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, - + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, - + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, @@ -143,17 +145,17 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, - + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, {{TK_forever, "forever"}, "\n a <= 1'b1;"}, {{TK_forever, "forever"}, " // Comment \n a <= 1'b1;"}, {{TK_forever, "forever"}, "\n // Comment\n a <= 1'b1;"}, - + {{TK_initial, "initial"}, " a = 1'b1;\n"}, {{TK_initial, "initial"}, "\n a = 1'b1;"}, {{TK_initial, "initial"}, " // Comment \n a = 1'b1;"}, {{TK_initial, "initial"}, "\n // Comment\n a = 1'b1;"}, - + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, {{TK_always_comb, "always_comb"}, "\n a = 1'b1;"}, {{TK_always_comb, "always_comb"}, " // Comment \n a = 1'b1;"}, @@ -163,15 +165,17 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always_latch, "always_latch"}, "\n a = 1'b1;"}, {{TK_always_latch, "always_latch"}, " // Comment \n a = 1'b1;"}, {{TK_always_latch, "always_latch"}, "\n // Comment\n a = 1'b1;"}, - + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, {{TK_always_ff, "always_ff"}, "@(a or b)\n a <= 1'b1;"}, - {{TK_always_ff, "always_ff"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always_ff, "always_ff"}, + " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, {{TK_always_ff, "always_ff"}, "@(a || b)\n // Comment\n a <= 1'b1;"}, - + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, {{TK_always, "always"}, "@(a or b)\n a = 1'b1;"}, - {{TK_always, "always"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always, "always"}, + " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, {{TK_always, "always"}, "@(a || b)\n // Comment\n <= 1'b1;"}, {{TK_always, "always"}, "@* a = 1'b1;"}, {{TK_always, "always"}, "@(*) a = 1'b1;"}, @@ -179,9 +183,241 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always, "always"}, " a = 1'b1;"}, {{TK_always, "always"}, " #10 a = 1'b1;"}, }; + RunLintTestCases(kTestCases); } +// Tests that rule can be disabled for if statements +TEST(ExplicitBeginRuleTest, AcceptsIfBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {"if (FOO) BAR"}, + {"else if (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "if_enable:false"); +} + +// Tests that rule can be disabled for else statements +TEST(ExplicitBeginRuleTest, AcceptsElseBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {"else \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "else_enable:false"); +} + +// Tests that rule can be disabled for for statements +TEST(ExplicitBeginRuleTest, AcceptsForBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {"for(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "for_enable:false"); +} + +// Tests that rule can be disabled for foreach statements +TEST(ExplicitBeginRuleTest, AcceptsForeachBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {"foreach(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "foreach_enable:false"); +} + +// Tests that rule can be disabled for while statements +TEST(ExplicitBeginRuleTest, AcceptsWhileBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {"while(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "while_enable:false"); +} + +// Tests that rule can be disabled for forever statements +TEST(ExplicitBeginRuleTest, AcceptsForeverBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {"forever a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "forever_enable:false"); +} + +// Tests that rule can be disabled for initial statements +TEST(ExplicitBeginRuleTest, AcceptsInitialBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {"initial a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "initial_enable:false"); +} + +// Tests that rule can be disabled for always_comb statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysCombBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {"always_comb a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_comb_enable:false"); +} + +// Tests that rule can be disabled for always_latch statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysLatchBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {"always_latch a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_latch_enable:false"); +} + +// Tests that rule can be disabled for always_ff statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysFFBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {"always_ff @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_ff_enable:false"); +} + +// Tests that rule can be disabled for always statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {"always @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_enable:false"); +} + } // namespace } // namespace analysis } // namespace verilog \ No newline at end of file From a8178d656217fe71329b6a32c7968e1bbd20c8e6 Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Mon, 16 May 2022 15:42:45 +0200 Subject: [PATCH 04/14] [LINT] Add rule explicit-begin Add rule to check if the begin keyword always follows a if/else/for. Related to #1321 --- verilog/analysis/checkers/BUILD | 33 ++++ .../analysis/checkers/explicit_begin_rule.cc | 160 ++++++++++++++++++ .../analysis/checkers/explicit_begin_rule.h | 88 ++++++++++ 3 files changed, 281 insertions(+) create mode 100644 verilog/analysis/checkers/explicit_begin_rule.cc create mode 100644 verilog/analysis/checkers/explicit_begin_rule.h diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index dc1177397..6d9293d5c 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -24,6 +24,7 @@ cc_library( ":disable-statement-rule", ":endif-comment-rule", ":enum-name-style-rule", + ":explicit-begin-rule", ":explicit-function-lifetime-rule", ":explicit-function-task-parameter-type-rule", ":explicit-parameter-storage-type-rule", @@ -1249,6 +1250,38 @@ cc_test( ], ) +cc_library( + name = "explicit-begin-rule", + srcs = ["explicit_begin_rule.cc"], + hdrs = ["explicit_begin_rule.h"], + deps = [ + "//common/analysis:lint-rule-status", + "//common/analysis:token-stream-lint-rule", + "//common/strings:comment-utils", + "//common/text:config-utils", + "//common/text:token-info", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/strings", + ], + alwayslink = 1, +) + +cc_test( + name = "explicit-begin-rule_test", + srcs = ["explicit_begin_rule_test.cc"], + deps = [ + ":explicit-begin-rule", + "//common/analysis:linter-test-utils", + "//common/analysis:token-stream-linter-test-utils", + "//common/text:symbol", + "//verilog/analysis:verilog-analyzer", + "//verilog/parser:verilog-token-enum", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "explicit-function-lifetime-rule", srcs = ["explicit_function_lifetime_rule.cc"], diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc new file mode 100644 index 000000000..690e381ee --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -0,0 +1,160 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/explicit_begin_rule.h" + +#include +#include +#include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/strings/comment_utils.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { + +using verible::AutoFix; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::TokenInfo; +using verible::TokenStreamLintRule; + +// Register the lint rule +VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); + +static const char kMessage[] = + "All block construct shall explicitely use begin/end"; + +const LintRuleDescriptor& ExplicitBeginRule::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "explicit-begin", + .topic = "explicit-begin", + .desc = + "Checks that a Verilog ``begin`` directive follows all " + "if, else and for loops.", + }; + return d; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo& token) { + // Responds to a token by updating the state of the analysis. + bool raise_violation = false; + switch (state_) { + case State::kNormal: { + // On if/else/for tokens; + // Also, skip all conditional statement after a for or a if + // Then, expect a `begin` token or record a violation. + switch (token.token_enum()) { + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_else: + last_condition_start_ = token; + end_of_condition_statement_ = token; + state_ = State::kInElse; + break; + default: + break; + } // switch (token) + break; + } + case State::kInElse: { + // If we are in a else statement, we can either have a if (and need to + // skip cond. statement) Or directly wait for a begin. We make use of the + // boolean raise_violation in order to avoid code duplication. + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + break; + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_begin: + state_ = State::kNormal; + break; + default: + raise_violation = true; + break; + } // switch (token) + break; + } + case State::kInCondition: { + if (token.text() == "(") { + condition_expr_level_++; + } else if (token.text() == ")") { + condition_expr_level_--; + if (condition_expr_level_ == 0) { + end_of_condition_statement_ = token; + state_ = State::kExpectBegin; + } + } + break; + } + case State::kExpectBegin: { + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + break; + case TK_begin: + // If we got our begin token, we go back to normal status + state_ = State::kNormal; + break; + default: { + raise_violation = true; + break; + } + } // switch (token) + break; + } + } // switch (state_) + + if (raise_violation) { + violations_.insert(LintViolation( + last_condition_start_, absl::StrCat(kMessage), + {AutoFix( + "Insert begin", + {end_of_condition_statement_, + absl::StrCat(end_of_condition_statement_.text(), " begin")})})); + + // Once the violation is raised, we go back to a normal, default, state + condition_expr_level_ = 0; + state_ = State::kNormal; + raise_violation = false; + } +} + +LintRuleStatus ExplicitBeginRule::Report() const { + return LintRuleStatus(violations_, GetDescriptor()); +} + +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h new file mode 100644 index 000000000..ad08a195f --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -0,0 +1,88 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ + +#include +#include +#include + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// Detects whether a Verilog `endif directive is followed by a comment that +// matches the opening `ifdef or `ifndef. +// +// Accepted examples: +// `ifdef FOO +// `endif // FOO +// +// `ifndef BAR +// `endif // BAR +// +// Rejected examples: +// `ifdef FOO +// `endif +// +// `ifdef FOO +// `endif // BAR +// +class ExplicitBeginRule : public verible::TokenStreamLintRule { + public: + using rule_type = verible::TokenStreamLintRule; + + static const LintRuleDescriptor& GetDescriptor(); + + ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} + + void HandleToken(const verible::TokenInfo& token) final; + + verible::LintRuleStatus Report() const final; + + private: + // States of the internal token-based analysis. + enum class State { + kNormal, + kInCondition, + kInElse, + kExpectBegin + }; + + // Internal lexical analysis state. + State state_; + + // Level of nested parenthesis when analizing conditional expressions. + int condition_expr_level_; + + // Token information for the last seen block opening (for/if/else). + verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); + verible::TokenInfo end_of_condition_statement_ = verible::TokenInfo::EOFToken(); + + + // Collection of found violations. + std::set violations_; + + void trigger_violation_(); +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_ENDIF_COMMENT_RULE_H_ From 65823cf7ea828595cd122779c25c91b8f7b5f547 Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Mon, 16 May 2022 16:32:22 +0200 Subject: [PATCH 05/14] [LINT] Add explicit-begin tests Closes #1336 --- .../analysis/checkers/explicit_begin_rule.cc | 6 +- .../analysis/checkers/explicit_begin_rule.h | 36 ++++--- .../checkers/explicit_begin_rule_test.cc | 101 ++++++++++++++++++ 3 files changed, 125 insertions(+), 18 deletions(-) create mode 100644 verilog/analysis/checkers/explicit_begin_rule_test.cc diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 690e381ee..3f38494cf 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -42,7 +42,7 @@ using verible::TokenStreamLintRule; VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); static const char kMessage[] = - "All block construct shall explicitely use begin/end"; + "All block construct shall explicitely use begin/end."; const LintRuleDescriptor& ExplicitBeginRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -89,6 +89,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { break; case TK_COMMENT_BLOCK: case TK_EOL_COMMENT: + case TK_NEWLINE: break; case TK_if: case TK_for: @@ -123,6 +124,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { break; case TK_COMMENT_BLOCK: case TK_EOL_COMMENT: + case TK_NEWLINE: break; case TK_begin: // If we got our begin token, we go back to normal status @@ -139,7 +141,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { if (raise_violation) { violations_.insert(LintViolation( - last_condition_start_, absl::StrCat(kMessage), + token, absl::StrCat(kMessage, " Expected begin, got ", token.text()), {AutoFix( "Insert begin", {end_of_condition_statement_, diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index ad08a195f..ae1d67406 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -31,18 +31,27 @@ namespace analysis { // matches the opening `ifdef or `ifndef. // // Accepted examples: -// `ifdef FOO -// `endif // FOO +// if (FOO) begin // -// `ifndef BAR -// `endif // BAR +// else begin +// +// else if (FOO) begin +// +// for (FOO) begin // // Rejected examples: -// `ifdef FOO -// `endif +// if (FOO) +// BAR +// +// else +// BAR +// +// else if (FOO) +// BAR +// +// for (FOO) begin +// BAR // -// `ifdef FOO -// `endif // BAR // class ExplicitBeginRule : public verible::TokenStreamLintRule { public: @@ -58,12 +67,7 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { private: // States of the internal token-based analysis. - enum class State { - kNormal, - kInCondition, - kInElse, - kExpectBegin - }; + enum class State { kNormal, kInCondition, kInElse, kExpectBegin }; // Internal lexical analysis state. State state_; @@ -73,8 +77,8 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { // Token information for the last seen block opening (for/if/else). verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); - verible::TokenInfo end_of_condition_statement_ = verible::TokenInfo::EOFToken(); - + verible::TokenInfo end_of_condition_statement_ = + verible::TokenInfo::EOFToken(); // Collection of found violations. std::set violations_; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc new file mode 100644 index 000000000..025678996 --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -0,0 +1,101 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/explicit_begin_rule.h" + +#include + +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/token_stream_linter_test_utils.h" +#include "common/text/symbol.h" +#include "gtest/gtest.h" +#include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunApplyFixCases; +using verible::RunLintTestCases; + +// Tests that space-only text passes. +TEST(ExplicitBeginRuleTest, AcceptsBlank) { + const std::initializer_list kTestCases = { + {""}, + {" "}, + {"\n"}, + {" \n\n"}, + }; + RunLintTestCases(kTestCases); +} + +// Tests that properly matched if/begin passes. +TEST(ExplicitBeginRuleTest, AcceptsEndifWithComment) { + const std::initializer_list kTestCases = { + {"if (FOO) begin"}, + {"if (FOO)\n begin"}, + {"if (FOO) //Comment\n begin"}, + {"else begin \n FOO"}, + {"else \nbegin \n FOO"}, + {"else //Comment\n begin \n FOO"}, + {"else \n //Comment\n begin \n FOO"}, + {"else if (FOO) begin"}, + {"else if (FOO)\n begin"}, + {"else if (FOO) //Comment\n begin"}, + {"else if (FOO)\n //Comment\n begin"}, + }; + RunLintTestCases(kTestCases); +} + +// Tests that unmatched block/begin fails is detected. +TEST(ExplicitBeginRuleTest, RejectsEndifWithoutComment) { + const std::initializer_list kTestCases = { + {"if (FOO) BAR"}, + {"if (FOO)\n BAR"}, + {"if (FOO) //Comment\n BAR"}, + {"else \n FOO"}, + {"else \n \n FOO"}, + {"else //Comment\n FOO"}, + {"else \n //Comment\n FOO"}, + {"else if (FOO) BAR"}, + {"else if (FOO)\n BAR"}, + {"else if (FOO) //Comment\n BAR"}, + {"else if (FOO)\n //Comment\n BAR"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(ExplicitBeginRuleTest, ApplyAutoFix) { + // Alternatives the auto fix offers + const std::initializer_list kTestCases = { + {"if (FOO) BAR","if (FOO) begin BAR"}, + {"if (FOO)\n BAR","if (FOO) begin\n BAR"}, + {"if (FOO) //Comment\n BAR","if (FOO) begin //Comment\n BAR"}, + {"else \n FOO","else begin \n FOO"}, + {"else \n \n FOO","else begin \n \n FOO"}, + {"else //Comment\n FOO","else begin //Comment\n FOO"}, + {"else \n //Comment\n FOO","else begin \n //Comment\n FOO"}, + {"else if (FOO) BAR","else if (FOO) begin BAR"}, + {"else if (FOO)\n BAR","else if (FOO) begin\n BAR"}, + {"else if (FOO) //Comment\n BAR","else if (FOO) begin //Comment\n BAR"}, + {"else if (FOO)\n //Comment\n BAR","else if (FOO) begin\n //Comment\n BAR"}, + }; + RunApplyFixCases(kTestCases, ""); +} + +} // namespace +} // namespace analysis +} // namespace verilog From 3178218ea37749023e60b64d32b1b7ba8e681a3e Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Tue, 31 May 2022 16:07:18 +0200 Subject: [PATCH 06/14] [LINT] Update explicit-begin tests Remove the fix suggestion as invalid --- .../checkers/explicit_begin_rule_test.cc | 66 +++++++------------ 1 file changed, 24 insertions(+), 42 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 025678996..6f64e40e8 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -43,59 +43,41 @@ TEST(ExplicitBeginRuleTest, AcceptsBlank) { } // Tests that properly matched if/begin passes. -TEST(ExplicitBeginRuleTest, AcceptsEndifWithComment) { +TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { const std::initializer_list kTestCases = { - {"if (FOO) begin"}, - {"if (FOO)\n begin"}, - {"if (FOO) //Comment\n begin"}, - {"else begin \n FOO"}, - {"else \nbegin \n FOO"}, - {"else //Comment\n begin \n FOO"}, - {"else \n //Comment\n begin \n FOO"}, - {"else if (FOO) begin"}, - {"else if (FOO)\n begin"}, - {"else if (FOO) //Comment\n begin"}, - {"else if (FOO)\n //Comment\n begin"}, + {"if (FOO) begin"}, + {"if (FOO)\n begin"}, + {"if (FOO) //Comment\n begin"}, + {"else begin \n FOO"}, + {"else \nbegin \n FOO"}, + {"else //Comment\n begin \n FOO"}, + {"else \n //Comment\n begin \n FOO"}, + {"else if (FOO) begin"}, + {"else if (FOO)\n begin"}, + {"else if (FOO) //Comment\n begin"}, + {"else if (FOO)\n //Comment\n begin"}, }; RunLintTestCases(kTestCases); } // Tests that unmatched block/begin fails is detected. -TEST(ExplicitBeginRuleTest, RejectsEndifWithoutComment) { +TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { const std::initializer_list kTestCases = { - {"if (FOO) BAR"}, - {"if (FOO)\n BAR"}, - {"if (FOO) //Comment\n BAR"}, - {"else \n FOO"}, - {"else \n \n FOO"}, - {"else //Comment\n FOO"}, - {"else \n //Comment\n FOO"}, - {"else if (FOO) BAR"}, - {"else if (FOO)\n BAR"}, - {"else if (FOO) //Comment\n BAR"}, - {"else if (FOO)\n //Comment\n BAR"}, + {{TK_if, "if"}, " (FOO) BAR"}, + {{TK_if, "if"}, " (FOO)\n BAR"}, + {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_else, "else"}, " \n \n FOO"}, + {{TK_else, "else"}, " //Comment\n FOO"}, + {{TK_else, "else"}, " \n //Comment\n FOO"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, }; RunLintTestCases(kTestCases); } -TEST(ExplicitBeginRuleTest, ApplyAutoFix) { - // Alternatives the auto fix offers - const std::initializer_list kTestCases = { - {"if (FOO) BAR","if (FOO) begin BAR"}, - {"if (FOO)\n BAR","if (FOO) begin\n BAR"}, - {"if (FOO) //Comment\n BAR","if (FOO) begin //Comment\n BAR"}, - {"else \n FOO","else begin \n FOO"}, - {"else \n \n FOO","else begin \n \n FOO"}, - {"else //Comment\n FOO","else begin //Comment\n FOO"}, - {"else \n //Comment\n FOO","else begin \n //Comment\n FOO"}, - {"else if (FOO) BAR","else if (FOO) begin BAR"}, - {"else if (FOO)\n BAR","else if (FOO) begin\n BAR"}, - {"else if (FOO) //Comment\n BAR","else if (FOO) begin //Comment\n BAR"}, - {"else if (FOO)\n //Comment\n BAR","else if (FOO) begin\n //Comment\n BAR"}, - }; - RunApplyFixCases(kTestCases, ""); -} - } // namespace } // namespace analysis } // namespace verilog From 35e3bbf818e6215196cb3cece2df114ec19f4036 Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Tue, 31 May 2022 16:08:40 +0200 Subject: [PATCH 07/14] [LINT] Remove fix suggestion also move the error anchor to offending if/else/for in order to allow test to pass --- verilog/analysis/checkers/explicit_begin_rule.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 3f38494cf..4e83d4524 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -141,11 +141,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { if (raise_violation) { violations_.insert(LintViolation( - token, absl::StrCat(kMessage, " Expected begin, got ", token.text()), - {AutoFix( - "Insert begin", - {end_of_condition_statement_, - absl::StrCat(end_of_condition_statement_.text(), " begin")})})); + last_condition_start_, absl::StrCat(kMessage, " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state condition_expr_level_ = 0; From 53c897655f26cbb48bbc8ec58f8ef06922a25222 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sat, 27 Apr 2024 00:09:33 +1000 Subject: [PATCH 08/14] Handle consecutive statements missing begin tokens --- .../analysis/checkers/explicit_begin_rule.cc | 47 ++++++++++++------- .../analysis/checkers/explicit_begin_rule.h | 2 + .../checkers/explicit_begin_rule_test.cc | 22 +++++++-- 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index fbf401688..542ddd600 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -138,24 +138,13 @@ bool ExplicitBeginRule::IsTokenEnabled(const TokenInfo &token) { } } -void ExplicitBeginRule::HandleToken(const TokenInfo &token) { - // Ignore all white space and comments and return immediately - switch (token.token_enum()) { - case TK_SPACE: - case TK_NEWLINE: - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - return; - default: - break; - } - +bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { case State::kNormal: { if (!IsTokenEnabled(token)) { - return; + break; } switch (token.token_enum()) { @@ -198,7 +187,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { break; } case State::kInAlways: { - // always is a little more complicated in that it can be imediattly + // always is a little more complicated in that it can be immediately // followed by a "begin" or followed by some special characters ("@" or // "*") and maybe a condition. switch (token.token_enum()) { @@ -209,7 +198,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { state_ = State::kNormal; break; case '(': - condition_expr_level_ = 1; + condition_expr_level_++; state_ = State::kInCondition; break; default: @@ -283,9 +272,33 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state - condition_expr_level_ = 0; state_ = State::kNormal; - raise_violation = false; + } + + return raise_violation; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Ignore all white space and comments and return immediately + switch (token.token_enum()) { + case TK_SPACE: + case TK_NEWLINE: + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + return; + default: + break; + } + + bool retry = HandleTokenStateMachine(token); + + // If this token raises a violation, it was because the statemachine was + // expecting a begin token. This token may expect a begin token too, so + // check it. + // Consider: forever if(a) #10; else #20; // 3 violations could be raised + // [forever, if, else]. + if (retry) { + HandleTokenStateMachine(token); } } diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 2117a1b24..005e20440 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -46,6 +46,8 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { verible::LintRuleStatus Report() const final; private: + bool HandleTokenStateMachine(const TokenInfo &token); + bool IsTokenEnabled(const TokenInfo &token); // States of the internal token-based analysis. diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 6c3bc5adb..3a99ce983 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -135,16 +135,20 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, + {{TK_for, "for"}, + "(i = 0; i < N; i++)\n", + {TK_if, "if"}, + " (FOO) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n // Comment\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) // Comment \n a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n // Comment\n a <= 1'b1;"}, {{TK_forever, "forever"}, " a <= 1'b1;\n"}, {{TK_forever, "forever"}, "\n a <= 1'b1;"}, @@ -182,6 +186,16 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always, "always"}, " @* a = 1'b1;"}, {{TK_always, "always"}, " a = 1'b1;"}, {{TK_always, "always"}, " #10 a = 1'b1;"}, + + // Multiple consecutive failures + {{TK_if, "if"}, "(FOO) ", {TK_for, "for"}, "(i = 0; i < N; i++) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_foreach, "foreach"}, "(array[i]) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_while, "while"}, "(i < N) i++;"}, + {"generate ", + {TK_if, "if"}, + "(FOO):g_foo\n", + {TK_always_comb, "always_comb\n"}, + " a = 1'b1;\n endgenerate"}, }; RunLintTestCases(kTestCases); From 97a253e6d11c4bdf32b7a30e91fb9b132e4877b5 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sat, 27 Apr 2024 00:09:33 +1000 Subject: [PATCH 09/14] Handle consecutive statements missing begin tokens --- .../analysis/checkers/explicit_begin_rule.cc | 47 +++++++++------ .../analysis/checkers/explicit_begin_rule.h | 2 + .../checkers/explicit_begin_rule_test.cc | 59 +++++++++++++++++-- 3 files changed, 87 insertions(+), 21 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index fbf401688..542ddd600 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -138,24 +138,13 @@ bool ExplicitBeginRule::IsTokenEnabled(const TokenInfo &token) { } } -void ExplicitBeginRule::HandleToken(const TokenInfo &token) { - // Ignore all white space and comments and return immediately - switch (token.token_enum()) { - case TK_SPACE: - case TK_NEWLINE: - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - return; - default: - break; - } - +bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { case State::kNormal: { if (!IsTokenEnabled(token)) { - return; + break; } switch (token.token_enum()) { @@ -198,7 +187,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { break; } case State::kInAlways: { - // always is a little more complicated in that it can be imediattly + // always is a little more complicated in that it can be immediately // followed by a "begin" or followed by some special characters ("@" or // "*") and maybe a condition. switch (token.token_enum()) { @@ -209,7 +198,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { state_ = State::kNormal; break; case '(': - condition_expr_level_ = 1; + condition_expr_level_++; state_ = State::kInCondition; break; default: @@ -283,9 +272,33 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state - condition_expr_level_ = 0; state_ = State::kNormal; - raise_violation = false; + } + + return raise_violation; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Ignore all white space and comments and return immediately + switch (token.token_enum()) { + case TK_SPACE: + case TK_NEWLINE: + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + return; + default: + break; + } + + bool retry = HandleTokenStateMachine(token); + + // If this token raises a violation, it was because the statemachine was + // expecting a begin token. This token may expect a begin token too, so + // check it. + // Consider: forever if(a) #10; else #20; // 3 violations could be raised + // [forever, if, else]. + if (retry) { + HandleTokenStateMachine(token); } } diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 2117a1b24..005e20440 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -46,6 +46,8 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { verible::LintRuleStatus Report() const final; private: + bool HandleTokenStateMachine(const TokenInfo &token); + bool IsTokenEnabled(const TokenInfo &token); // States of the internal token-based analysis. diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 6c3bc5adb..35c33aae7 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -110,6 +110,18 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always @* begin a = 1'b1;"}, {"always begin a = 1'b1;"}, {"always begin #10 a = 1'b1;"}, + + // Multiple consecutive failures + {"if(FOO) begin for(i = 0; i < N; i++) begin a <= i;"}, + {"if(FOO) begin foreach(array[i]) begin a <= i;"}, + {"if(FOO) begin while(i < N) begin i++;"}, + {"always @* begin if(FOO) begin a = 1; end else begin a = 0;"}, + {"always @(*) begin if(FOO) begin a = 1; end else begin a = 0;"}, + {"always @(posedge c) begin if(FOO) begin a <= 1; end else begin " + "a <= 0;"}, + {"always_comb begin if(FOO) begin a = 1; end else begin a = 0;"}, + {"always_ff @(posedge c) begin if(FOO) begin a <= 1; end else begin " + "a <= 0;"}, }; RunLintTestCases(kTestCases); @@ -135,16 +147,20 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, + {{TK_for, "for"}, + "(i = 0; i < N; i++)\n", + {TK_if, "if"}, + " (FOO) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n // Comment\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) // Comment \n a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n // Comment\n a <= 1'b1;"}, {{TK_forever, "forever"}, " a <= 1'b1;\n"}, {{TK_forever, "forever"}, "\n a <= 1'b1;"}, @@ -182,6 +198,41 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always, "always"}, " @* a = 1'b1;"}, {{TK_always, "always"}, " a = 1'b1;"}, {{TK_always, "always"}, " #10 a = 1'b1;"}, + + // Multiple consecutive failures + {{TK_if, "if"}, "(FOO) ", {TK_for, "for"}, "(i = 0; i < N; i++) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_foreach, "foreach"}, "(array[i]) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_while, "while"}, "(i < N) i++;"}, + {{TK_always, "always"}, + " @* ", + {TK_if, "if"}, + "(FOO) a = 1;", + {TK_else, "else"}, + " a = 0;"}, + {{TK_always, "always"}, + " @(*) ", + {TK_if, "if"}, + "(FOO) a = 1;", + {TK_else, "else"}, + " a = 0;"}, + {{TK_always, "always"}, + " @(posedge c) ", + {TK_if, "if"}, + "(FOO) a <= 1;", + {TK_else, "else"}, + " a <= 0;"}, + {{TK_always_comb, "always_comb"}, + " ", + {TK_if, "if"}, + "(FOO) a = 1;", + {TK_else, "else"}, + " a = 0;"}, + {{TK_always_ff, "always_ff"}, + " @(posedge c) ", + {TK_if, "if"}, + "(FOO) a <= 1;", + {TK_else, "else"}, + " a <= 0;"}, }; RunLintTestCases(kTestCases); From 40b1497157ab219a52b5200ad5f70f4ec23bbd5b Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Tue, 30 Apr 2024 23:44:00 +1000 Subject: [PATCH 10/14] Fixed and added lint tool tests --- verilog/tools/lint/BUILD | 1 + verilog/tools/lint/testdata/always_comb_blocking.sv | 3 ++- verilog/tools/lint/testdata/always_ff_non_blocking.sv | 3 ++- verilog/tools/lint/testdata/explicit_begin.sv | 4 ++++ verilog/tools/lint/testdata/generate_begin_module.sv | 4 +++- verilog/tools/lint/testdata/plusarg_assignment.sv | 4 +++- verilog/tools/lint/testdata/suspicious_semicolon.sv | 1 + 7 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 verilog/tools/lint/testdata/explicit_begin.sv diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 75e2ca46a..aad9e55bd 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -61,6 +61,7 @@ _linter_test_configs = [ ("disable-statement", "disable_statement", False), ("endif-comment", "endif_comment", False), ("enum-name-style", "enum_name_style", True), + ("explicit-begin", "explicit_begin", False), ("explicit-function-lifetime", "explicit_function_lifetime", True), ("explicit-function-task-parameter-type", "explicit_function_parameter_type", True), ("explicit-function-task-parameter-type", "explicit_task_parameter_type", True), diff --git a/verilog/tools/lint/testdata/always_comb_blocking.sv b/verilog/tools/lint/testdata/always_comb_blocking.sv index 403cf28fb..e54eb4376 100644 --- a/verilog/tools/lint/testdata/always_comb_blocking.sv +++ b/verilog/tools/lint/testdata/always_comb_blocking.sv @@ -1,4 +1,5 @@ module always_comb_blocking; - always_comb + always_comb begin a <= b; // [Style: combinational-logic] [always-comb-blocking] + end endmodule diff --git a/verilog/tools/lint/testdata/always_ff_non_blocking.sv b/verilog/tools/lint/testdata/always_ff_non_blocking.sv index a699fb8ba..0a10182b5 100644 --- a/verilog/tools/lint/testdata/always_ff_non_blocking.sv +++ b/verilog/tools/lint/testdata/always_ff_non_blocking.sv @@ -1,4 +1,5 @@ module always_ff_non_blocking; - always_ff @(posedge c) + always_ff @(posedge c) begin a = b; // [Style: sequential-logic] [always-ff-non-blocking] + end endmodule diff --git a/verilog/tools/lint/testdata/explicit_begin.sv b/verilog/tools/lint/testdata/explicit_begin.sv new file mode 100644 index 000000000..3aadad445 --- /dev/null +++ b/verilog/tools/lint/testdata/explicit_begin.sv @@ -0,0 +1,4 @@ +module explicit_begin (); + always_comb + a = 1; +endmodule diff --git a/verilog/tools/lint/testdata/generate_begin_module.sv b/verilog/tools/lint/testdata/generate_begin_module.sv index 9cd984845..5ed49e144 100644 --- a/verilog/tools/lint/testdata/generate_begin_module.sv +++ b/verilog/tools/lint/testdata/generate_begin_module.sv @@ -2,7 +2,9 @@ module generate_begin_module; // verilog_lint: waive legacy-generate-region generate begin : gen_block1 - always @(posedge clk) foo <= bar; + always @(posedge clk) begin + foo <= bar; + end end endgenerate endmodule diff --git a/verilog/tools/lint/testdata/plusarg_assignment.sv b/verilog/tools/lint/testdata/plusarg_assignment.sv index 89f9f2435..af2210f93 100644 --- a/verilog/tools/lint/testdata/plusarg_assignment.sv +++ b/verilog/tools/lint/testdata/plusarg_assignment.sv @@ -1,6 +1,8 @@ class foo; function bar; // The use of $test$plusargs() is not allowed. - if ($test$plusargs("baz")) return 1; + if ($test$plusargs("baz")) begin + return 1; + end endfunction endclass diff --git a/verilog/tools/lint/testdata/suspicious_semicolon.sv b/verilog/tools/lint/testdata/suspicious_semicolon.sv index ab49fb6cc..45697b1ff 100644 --- a/verilog/tools/lint/testdata/suspicious_semicolon.sv +++ b/verilog/tools/lint/testdata/suspicious_semicolon.sv @@ -1,5 +1,6 @@ module suspicious_semicolon (); initial begin + // verilog_lint: waive explicit-begin if (x); $display("Hi"); end From 06bfc9338ca78486d4e3d29f5501ef9b6083b2a9 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Wed, 19 Jun 2024 22:40:12 +1000 Subject: [PATCH 11/14] Constraints can have if-else and foreach statements, but use curly braces instead of begin-end. To keep things simple, this change ignores all statements inside a constraint. --- .../analysis/checkers/explicit_begin_rule.cc | 49 +++++++++++++++++++ .../analysis/checkers/explicit_begin_rule.h | 15 +++++- .../checkers/explicit_begin_rule_test.cc | 19 +++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index e8faef13a..56fdf5d3a 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -137,6 +137,20 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { bool raise_violation = false; switch (state_) { case State::kNormal: { + // Special handling for constraints + switch (token.token_enum()) { + case TK_constraint: { + constraint_expr_level_ = 0; + state_ = State::kConstraint; + return false; + } + case TK_with: { + constraint_expr_level_ = 0; + state_ = State::kInlineConstraint; + return false; + } + } + if (!IsTokenEnabled(token)) { break; } @@ -258,6 +272,41 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { } break; } + case State::kInlineConstraint: { + switch (token.token_enum()) { + case '{': { + // An InlineConstraint + constraint_expr_level_++; + state_ = State::kConstraint; + break; + } + default: { + // throw away everything else + state_ = State::kNormal; + break; + } + } + } + case State::kConstraint: { + // System Verilog constraints are special and use curly braces {} + // instead of begin-end. So ignore all constraints + switch (token.token_enum()) { + case '{': { + constraint_expr_level_++; + break; + } + case '}': { + constraint_expr_level_--; + if (constraint_expr_level_ == 0) { + state_ = State::kNormal; + } + } + default: { + // throw away everything else + break; + } + } + } } // switch (state_) if (raise_violation) { diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 4b41b2f09..5604e8e63 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -51,14 +51,25 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { bool IsTokenEnabled(const TokenInfo &token); // States of the internal token-based analysis. - enum class State { kNormal, kInAlways, kInCondition, kInElse, kExpectBegin }; + enum class State { + kNormal, + kInAlways, + kInCondition, + kInElse, + kExpectBegin, + kConstraint, + kInlineConstraint + }; // Internal lexical analysis state. State state_; - // Level of nested parenthesis when analysing conditional expressions. + // Level of nested parenthesis when analysing conditional expressions int condition_expr_level_; + // Level inside a constraint expression + int constraint_expr_level_; + // Configuration bool if_enable_ = true; bool else_enable_ = true; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index c9ab445ba..960e833a7 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -111,6 +111,16 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always begin a = 1'b1;"}, {"always begin #10 a = 1'b1;"}, + // Ignore constraints + {"constraint c_array { foreach (array[i]) {array[i] == i;}}"}, + {"constraint c {if(a == 2){b == 1;}else{b == 2;}}"}, + + // Ignore inline constraints + {"task a(); std::randomize(b) with {foreach(b[i]){b[i] inside " + "{[0:1024]};}}; endtask"}, + {"task a(); std::randomize(b) with {if(a == 2){b == 1;}else{b == 2;}}; " + "endtask"}, + // Multiple consecutive failures {"if(FOO) begin for(i = 0; i < N; i++) begin a <= i;"}, {"if(FOO) begin foreach(array[i]) begin a <= i;"}, @@ -123,6 +133,10 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always_comb begin if(FOO) begin a = 1; end else begin a = 0;"}, {"always_ff @(posedge c) begin if(FOO) begin a <= 1; end else begin " "a <= 0;"}, + {"constraint c_array { foreach (array[i]) {array[i] == i;}}if(FOO) begin " + "a <= 1;end"}, + {"if(FOO) begin a <= 1;end constraint c {if(a == 2){b == 1;}else{b == " + "2;}}"}, }; RunLintTestCases(kTestCases); @@ -234,6 +248,11 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { "(FOO) a <= 1;", {TK_else, "else"}, " a <= 0;"}, + {"constraint c_array { foreach (array[i]) array[i] == i;}", + {TK_if, "if"}, + "(FOO) a <= 1;"}, + {{TK_if, "if"}, + "(FOO) a <= 1; constraint c {if(a == 2) b == 1;else b == 2;}"}, }; RunLintTestCases(kTestCases); From 6da2fe5fc22fc13a4f58220b811b5abb544ce510 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Wed, 4 Sep 2024 23:34:29 +1000 Subject: [PATCH 12/14] Fixing blant, ClangTidy and fallthrough compile errors --- verilog/analysis/checkers/BUILD | 6 ++-- .../analysis/checkers/explicit_begin_rule.cc | 30 +++++++++++++--- .../analysis/checkers/explicit_begin_rule.h | 36 +++++++++---------- .../checkers/explicit_begin_rule_test.cc | 2 -- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index d5a59c728..760e7d538 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1261,13 +1261,15 @@ cc_library( deps = [ "//common/analysis:lint-rule-status", "//common/analysis:token-stream-lint-rule", - "//common/strings:comment-utils", "//common/text:config-utils", "//common/text:token-info", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", + "@com_google_absl//absl/strings:string_view", ], alwayslink = 1, ) @@ -1279,9 +1281,9 @@ cc_test( ":explicit-begin-rule", "//common/analysis:linter-test-utils", "//common/analysis:token-stream-linter-test-utils", - "//common/text:symbol", "//verilog/analysis:verilog-analyzer", "//verilog/parser:verilog-token-enum", + "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], ) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 56fdf5d3a..44fee0531 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -14,16 +14,14 @@ #include "verilog/analysis/checkers/explicit_begin_rule.h" -#include #include -#include -#include +#include "absl/base/attributes.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/token_stream_lint_rule.h" -#include "common/strings/comment_utils.h" #include "common/text/config_utils.h" #include "common/text/token_info.h" #include "verilog/analysis/descriptions.h" @@ -33,7 +31,6 @@ namespace verilog { namespace analysis { -using verible::AutoFix; using verible::LintRuleStatus; using verible::LintViolation; using verible::TokenInfo; @@ -149,6 +146,10 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { state_ = State::kInlineConstraint; return false; } + default: { + // Do nothing + break; + } } if (!IsTokenEnabled(token)) { @@ -158,8 +159,11 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { switch (token.token_enum()) { // After token expect "begin" case TK_always_comb: + ABSL_FALLTHROUGH_INTENDED; case TK_always_latch: + ABSL_FALLTHROUGH_INTENDED; case TK_forever: + ABSL_FALLTHROUGH_INTENDED; case TK_initial: start_token_ = token; state_ = State::kExpectBegin; @@ -168,9 +172,13 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // be tokens prior to the condition (like in an "always_ff" statement) // and these are all ignored. case TK_if: + ABSL_FALLTHROUGH_INTENDED; case TK_always_ff: + ABSL_FALLTHROUGH_INTENDED; case TK_for: + ABSL_FALLTHROUGH_INTENDED; case TK_foreach: + ABSL_FALLTHROUGH_INTENDED; case TK_while: condition_expr_level_ = 0; start_token_ = token; @@ -200,6 +208,7 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // "*") and maybe a condition. switch (token.token_enum()) { case '@': + break; case '*': break; case TK_begin: @@ -250,6 +259,7 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { if (condition_expr_level_ == 0) { state_ = State::kExpectBegin; } + break; } default: { // throw away everything else @@ -287,6 +297,7 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { } } } + ABSL_FALLTHROUGH_INTENDED; case State::kConstraint: { // System Verilog constraints are special and use curly braces {} // instead of begin-end. So ignore all constraints @@ -300,12 +311,18 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { if (constraint_expr_level_ == 0) { state_ = State::kNormal; } + break; } default: { // throw away everything else break; } } + break; + } + default: { + // Do nothing + break; } } // switch (state_) @@ -325,8 +342,11 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // Ignore all white space and comments and return immediately switch (token.token_enum()) { case TK_SPACE: + return; case TK_NEWLINE: + return; case TK_COMMENT_BLOCK: + return; case TK_EOL_COMMENT: return; default: diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 5604e8e63..a91205a45 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -16,9 +16,9 @@ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ #include -#include -#include +#include "absl/status/status.h" +#include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/token_stream_lint_rule.h" #include "common/text/token_info.h" @@ -39,8 +39,6 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { absl::Status Configure(absl::string_view configuration) final; - ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} - void HandleToken(const verible::TokenInfo &token) final; verible::LintRuleStatus Report() const final; @@ -62,29 +60,29 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { }; // Internal lexical analysis state. - State state_; + State state_{State::kNormal}; // Level of nested parenthesis when analysing conditional expressions - int condition_expr_level_; + int condition_expr_level_{0}; // Level inside a constraint expression - int constraint_expr_level_; + int constraint_expr_level_{0}; // Configuration - bool if_enable_ = true; - bool else_enable_ = true; - bool always_enable_ = true; - bool always_comb_enable_ = true; - bool always_latch_enable_ = true; - bool always_ff_enable_ = true; - bool for_enable_ = true; - bool forever_enable_ = true; - bool foreach_enable_ = true; - bool while_enable_ = true; - bool initial_enable_ = true; + bool if_enable_{true}; + bool else_enable_{true}; + bool always_enable_{true}; + bool always_comb_enable_{true}; + bool always_latch_enable_{true}; + bool always_ff_enable_{true}; + bool for_enable_{true}; + bool forever_enable_{true}; + bool foreach_enable_{true}; + bool while_enable_{true}; + bool initial_enable_{true}; // Token that requires blocking statement. - verible::TokenInfo start_token_ = verible::TokenInfo::EOFToken(); + verible::TokenInfo start_token_{verible::TokenInfo::EOFToken()}; // Collection of found violations. std::set violations_; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 960e833a7..50d0d6683 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -18,7 +18,6 @@ #include "common/analysis/linter_test_utils.h" #include "common/analysis/token_stream_linter_test_utils.h" -#include "common/text/symbol.h" #include "gtest/gtest.h" #include "verilog/analysis/verilog_analyzer.h" #include "verilog/parser/verilog_token_enum.h" @@ -28,7 +27,6 @@ namespace analysis { namespace { using verible::LintTestCase; -using verible::RunApplyFixCases; using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; From 93c946546cac012ea1cf232d78b9221a7fe461bb Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Wed, 18 Sep 2024 19:23:18 -0700 Subject: [PATCH 13/14] Use latest bant for dependency checking. --- .github/workflows/verible-ci.yml | 33 +++++++++++++------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml index 1a10ceb04..5202674bb 100644 --- a/.github/workflows/verible-ci.yml +++ b/.github/workflows/verible-ci.yml @@ -115,7 +115,7 @@ jobs: name: "diag" path: "**/plot_*.svg" - RunBantBuildCleaneer: + RunBantBuildCleaner: # Running http://bant.build/ to check all dependencies in BUILD files. runs-on: ubuntu-24.04 steps: @@ -124,28 +124,10 @@ jobs: with: fetch-depth: 0 - - name: Build Project genrules - run: | - # Fetch all dependency and run genrules for bant to see every file - # that makes it into the compile. - bazel fetch ... - bazel build \ - //common/analysis:command-file-lexer \ - //common/lsp:jcxxgen-testfile-gen \ - //common/lsp:lsp-protocol-gen \ - //common/util:version-header \ - //verilog/CST:verilog-nonterminals-foreach-gen \ - //verilog/parser:gen-verilog-token-enum \ - //verilog/parser:verilog-lex \ - //verilog/parser:verilog-parse-interface \ - //verilog/parser:verilog-y \ - //verilog/parser:verilog-y-final \ - //verilog/tools/kythe:verilog-extractor-indexing-fact-type-foreach-gen - - name: Get Bant run: | # TODO: provide this as action where we simply say with version=... - VERSION="v0.1.5" + VERSION="v0.1.7" STATIC_VERSION="bant-${VERSION}-linux-static-x86_64" wget "https://github.com/hzeller/bant/releases/download/${VERSION}/${STATIC_VERSION}.tar.gz" tar xvzf "${STATIC_VERSION}.tar.gz" @@ -153,6 +135,17 @@ jobs: ln -sf ../"${STATIC_VERSION}/bin/bant" bin/ bin/bant -V + - name: Build Project genrules + run: | + # Fetch all dependencies and run genrules for bant to see every file + # that makes it into the compile. Use bant itself to find genrules. + bazel fetch ... + bazel build $(bin/bant -q genrule-outputs | awk '{print $2}') \ + //common/analysis:command-file-lexer \ + //verilog/parser:verilog-lex \ + //verilog/parser:verilog-y \ + //verilog/parser:verilog-y-final + - name: Run bant build-cleaner run: | bin/bant dwyu ... From ff575c8eedbcd8a18214b066ae4520bd19144db3 Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Thu, 19 Sep 2024 17:33:29 -0700 Subject: [PATCH 14/14] Update to latest run-clang-tidy-cached --- .github/bin/run-clang-tidy-cached.cc | 115 ++++++++++++++++++--------- 1 file changed, 78 insertions(+), 37 deletions(-) diff --git a/.github/bin/run-clang-tidy-cached.cc b/.github/bin/run-clang-tidy-cached.cc index 8f4e7d6d8..d0d48e1be 100755 --- a/.github/bin/run-clang-tidy-cached.cc +++ b/.github/bin/run-clang-tidy-cached.cc @@ -15,8 +15,7 @@ B=${0%%.cc}; [ "$B" -nt "$0" ] || c++ -std=c++17 -o"$B" "$0" && exec "$B" "$@"; // See the License for the specific language governing permissions and // limitations under the License. -// Location: https://github.com/hzeller/dev-tools -// Last update: 2024-09-16 cf86e3b6c51dbf620a08fb2a59f6ea71b6bebe3d +// Location: https://github.com/hzeller/dev-tools (2024-09-19) // Script to run clang-tidy on files in a bazel project while caching the // results as clang-tidy can be pretty slow. The clang-tidy output messages @@ -62,18 +61,19 @@ B=${0%%.cc}; [ "$B" -nt "$0" ] || c++ -std=c++17 -o"$B" "$0" && exec "$B" "$@"; // Dont' change anything here, override for your project below in kConfig. struct ConfigValues { // Prefix for the cache to write to. If empty, use name of toplevel directory. + // Typical would be name of project with underscore suffix. std::string_view cache_prefix; // Directory to start recurse for sources createing a file list. - // Some projects need e.g. "src/". + // Some projects e.g. start at "src/". std::string_view start_dir; - // Regular expression matching files that should be included from file list. + // Regular expression matching files that should be included in file list. std::string_view file_include_re = ".*"; // Regular expxression matching files that should be excluded from file list. - // If overriding, make sure to include at least ".git". - std::string_view file_exclude_re = ".git/|.github/"; + // If searching from toplevel, make sure to include at least ".git/". + std::string_view file_exclude_re = "^(\\.git|\\.github|build)/"; // A file in the toplevel of the project that should exist, typically // something used to set up the build environment, such as MODULE.bazel, @@ -86,6 +86,11 @@ struct ConfigValues { // (Default configuration: just .clang-tidy as this should always be there) std::string_view toplevel_build_file = ".clang-tidy"; + // Bazel projects have some complicated paths where generated and external + // sources reside. Setting this to true will tell this script canonicalize + // these in the output. Set to true if this is a bazel project. + bool is_bazel_project = false; + // If the compilation DB or toplevel_build_file changed in timestamp, it // might be worthwhile revisiting sources that previously had issues. // This flag enables that. @@ -94,6 +99,14 @@ struct ConfigValues { // few problematic sources to begin with, otherwise every update of the // compilation DB will re-trigger revisiting all of them. bool revisit_brokenfiles_if_build_config_newer = true; + + // Revisit a source file if any of its include files changed content. Say + // foo.cc includes bar.h. Reprocess foo.cc with clang-tidy when bar.h changed, + // even if foo.cc is unchanged. This will find issues in which foo.cc relies + // on something bar.h provides. + // Usually good to keep on, but it can result in situations in which a header + // that is included by a lot of other files results in lots of reprocessing. + bool revisit_if_any_include_changes = true; }; // --------------[ Project-specific configuration ]-------------- @@ -101,15 +114,27 @@ static constexpr ConfigValues kConfig = { .cache_prefix = "verible_", .file_exclude_re = ".git/|.github/" // stuff we're not interested in "|vscode/" // some generated code in there - "|tree_operations_test" // very slow - "|symbol_table_test", + "|tree_operations_test" // very slow to process. + "|symbol_table_test", // ... .toplevel_build_file = "MODULE.bazel", + .is_bazel_project = true, }; // -------------------------------------------------------------- -// Files to be considered. +// Files that look like relevant include files. +inline bool IsIncludeExtension(const std::string &extension) { + for (const std::string_view e : {".h", ".hpp", ".hxx", ".inl"}) { + if (extension == e) return true; + } + return false; +} + +// Filter for source files to be considered. inline bool ConsiderExtension(const std::string &extension) { - return extension == ".cc" || extension == ".cpp" || extension == ".h"; + for (const std::string_view e : {".cc", ".cpp", ".cxx"}) { + if (extension == e) return true; + } + return IsIncludeExtension(extension); } // Configuration of clang-tidy itself. @@ -289,6 +314,10 @@ class ClangTidyRunner { // Use major version as part of name of our configuration specific dir. auto version = GetCommandOutput(clang_tidy_ + " --version"); + if (version.empty()) { + std::cerr << "Could not invoke " << clang_tidy_ << "; is it in PATH ?\n"; + exit(EXIT_FAILURE); + } std::smatch version_match; const std::string major_version = std::regex_search(version, version_match, std::regex{"version ([0-9]+)"}) @@ -303,16 +332,18 @@ class ClangTidyRunner { } // Fix filename paths found in logfiles that are not emitted relative to - // project root in the log (bazel has its own, so this fixes bazel-specific - // projects) + // project root in the log - remove that prefix. + // (bazel has its own, so if this is bazel, also bazel-specific fix up that). static void RepairFilenameOccurences(const fs::path &infile, const fs::path &outfile) { static const std::regex sFixPathsRe = []() { std::string canonicalize_expr = "(^|\\n)("; // fix names at start of line - auto root = GetCommandOutput("bazel info execution_root 2>/dev/null"); - if (!root.empty()) { - root.pop_back(); // remove newline. - canonicalize_expr += root + "/|"; + if (kConfig.is_bazel_project) { + auto bzroot = GetCommandOutput("bazel info execution_root 2>/dev/null"); + if (!bzroot.empty()) { + bzroot.pop_back(); // remove newline. + canonicalize_expr += bzroot + "/|"; + } } canonicalize_expr += fs::current_path().string() + "/"; // $(pwd)/ canonicalize_expr += @@ -362,34 +393,41 @@ class FileGatherer { } // Remember content hash of header, so that we can make changed headers // influence the hash of a file including this. - if (extension == ".h") { + if (kConfig.revisit_if_any_include_changes && + IsIncludeExtension(extension)) { // Since the files might be included sloppily without prefix path, // just keep track of the basename (but since there might be collisions, // accomodate all of them by xor-ing the hashes). - const std::string just_basename = fs::path(file).filename(); + const std::string just_basename = p.filename(); header_hashes[just_basename] ^= hashContent(GetContent(p)); } } std::cerr << files_of_interest_.size() << " files of interest.\n"; - // Create content hash address. If any header a file depends on changes, we - // want to reprocess. So we make the hash dependent on header content as - // well. + // Create content hash address for the cache and build list of work items. + // If we want to revisit if headers changed, make hash dependent on them. std::list work_queue; - const std::regex inc_re("\"([0-9a-zA-Z_/-]+\\.h)\""); // match include - for (filepath_contenthash_t &f : files_of_interest_) { - const auto content = GetContent(f.first); - f.second = hashContent(content); - for (ReIt it(content.begin(), content.end(), inc_re); it != ReIt(); - ++it) { - const std::string &header_path = (*it)[1].str(); - const std::string header_basename = fs::path(header_path).filename(); - f.second ^= header_hashes[header_basename]; + const std::regex inc_re( + R"""(#\s*include\s+"([0-9a-zA-Z_/-]+\.[a-zA-Z]+)")"""); + for (filepath_contenthash_t &work_file : files_of_interest_) { + const auto content = GetContent(work_file.first); + work_file.second = hashContent(content); + if (kConfig.revisit_if_any_include_changes) { + // Update the hash with all the hashes from all include files. + for (ReIt it(content.begin(), content.end(), inc_re); it != ReIt(); + ++it) { + const std::string &header_path = (*it)[1].str(); + const std::string header_basename = fs::path(header_path).filename(); + const auto found = header_hashes.find(header_basename); + if (found != header_hashes.end()) { + work_file.second ^= found->second; + } + } } - // Recreate if we don't have it yet or if it contains messages but is - // older than WORKSPACE or compilation db. Maybe something got fixed. - if (store_.NeedsRefresh(f, min_freshness)) { - work_queue.emplace_back(f); + // Recreate if we don't have it yet or if it contains findings but is + // older than build environment. Maybe something got fixed: revisit file. + if (store_.NeedsRefresh(work_file, min_freshness)) { + work_queue.emplace_back(work_file); } } return work_queue; @@ -397,11 +435,14 @@ class FileGatherer { // Tally up findings for files of interest and assemble in one file. // (BuildWorkList() needs to be called first). - size_t CreateReport(const fs::path &project_dir, + size_t CreateReport(const fs::path &cache_dir, std::string_view symlink_detail, std::string_view symlink_summary) const { - const fs::path tidy_outfile = project_dir / "tidy.out"; - const fs::path tidy_summary = project_dir / "tidy-summary.out"; + // Make it possible to keep independent reports for different invocation + // locations (e.g. two checkouts of the same project) using the same cache. + const std::string suffix = ToHex(hashContent(fs::current_path().string())); + const fs::path tidy_outfile = cache_dir / ("tidy.out-" + suffix); + const fs::path tidy_summary = cache_dir / ("tidy-summary.out-" + suffix); // Assemble the separate outputs into a single file. Tally up per-check const std::regex check_re("(\\[[a-zA-Z.-]+\\])\n");