From 095cd2ec44a11e9f5923c6e19831d2751fc935dd Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sat, 23 Nov 2024 11:29:58 -0800 Subject: [PATCH 1/5] Use different clang tool versions for different taskss. * clang-format : use version 17, as 18 seems to have slight differences between microversions. * clang-tidy: use latest 18 --- shell.nix | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/shell.nix b/shell.nix index 51449ec12..abbbf51aa 100644 --- a/shell.nix +++ b/shell.nix @@ -38,6 +38,15 @@ verible_used_stdenv.mkDerivation { lcov # coverage html generation. bazel-buildtools # buildifier - clang-tools_17 # For clang-tidy; clangd + clang-tools_18 # for clang-tidy + clang-tools_17 # for clang-format ]; + shellHook = '' + # clang tidy: use latest. + export CLANG_TIDY=${pkgs.clang-tools_18}/bin/clang-tidy + + # There is too much volatility between even micro-versions of + # clang-format 18. Let's use 17 for now. + export CLANG_FORMAT=${pkgs.clang-tools_17}/bin/clang-format + ''; } From a2dc3039317278406a91ba8a4badd3853a2dbdde Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sat, 23 Nov 2024 11:32:40 -0800 Subject: [PATCH 2/5] Fix some non-const global variables. In one case, a global variable was even assigned, fixed that. --- verible/common/formatting/state-node.cc | 2 +- .../checkers/macro-name-style-rule.cc | 14 ++++----- .../proper-parameter-declaration-rule.cc | 31 +++++++++++-------- .../proper-parameter-declaration-rule.h | 7 +++++ 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/verible/common/formatting/state-node.cc b/verible/common/formatting/state-node.cc index d13165f1f..7bba9322c 100644 --- a/verible/common/formatting/state-node.cc +++ b/verible/common/formatting/state-node.cc @@ -32,7 +32,7 @@ namespace verible { -static absl::string_view kNotForAlignment = +static constexpr absl::string_view kNotForAlignment = "Aligned tokens should never use line-wrap optimization!"; static SpacingDecision FrontTokenSpacing(const FormatTokenRange range) { diff --git a/verible/verilog/analysis/checkers/macro-name-style-rule.cc b/verible/verilog/analysis/checkers/macro-name-style-rule.cc index 36267a631..3ff7ddbce 100644 --- a/verible/verilog/analysis/checkers/macro-name-style-rule.cc +++ b/verible/verilog/analysis/checkers/macro-name-style-rule.cc @@ -47,16 +47,16 @@ static constexpr absl::string_view kUVMLowerCaseMessage = static constexpr absl::string_view kUVMUpperCaseMessage = "'UVM_*' named macros must follow 'UPPER_SNAKE_CASE' format."; -static absl::string_view lower_snake_case_regex = "[a-z_0-9]+"; -static absl::string_view upper_snake_case_regex = "[A-Z_0-9]+"; +static constexpr absl::string_view kLowerSnakeCaseRegex = "[a-z_0-9]+"; +static constexpr absl::string_view kUpperSnakeCaseRegex = "[A-Z_0-9]+"; MacroNameStyleRule::MacroNameStyleRule() : style_regex_( - std::make_unique(upper_snake_case_regex, re2::RE2::Quiet)), + std::make_unique(kUpperSnakeCaseRegex, re2::RE2::Quiet)), style_lower_snake_case_regex_( - std::make_unique(lower_snake_case_regex, re2::RE2::Quiet)), - style_upper_snake_case_regex_(std::make_unique( - upper_snake_case_regex, re2::RE2::Quiet)) {} + std::make_unique(kLowerSnakeCaseRegex, re2::RE2::Quiet)), + style_upper_snake_case_regex_( + std::make_unique(kUpperSnakeCaseRegex, re2::RE2::Quiet)) {} const LintRuleDescriptor &MacroNameStyleRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -70,7 +70,7 @@ const LintRuleDescriptor &MacroNameStyleRule::GetDescriptor() { "and \"UPPER_SNAKE_CASE\" naming conventions respectively. Refer to " "https://github.com/chipsalliance/verible/tree/master/verilog/tools/" "lint#readme for more detail on verible regex patterns.", - .param = {{"style_regex", std::string(upper_snake_case_regex), + .param = {{"style_regex", std::string(kUpperSnakeCaseRegex), "A regex used to check macro names style."}}, }; return d; diff --git a/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.cc b/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.cc index db2f4b54e..f58dbda3e 100644 --- a/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.cc +++ b/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.cc @@ -56,12 +56,9 @@ static constexpr absl::string_view kLocalParamAllowPackageMessage = "\'localparam\' declarations should only be within modules, packages or " "class definition bodies."; -static absl::string_view kParameterMessage = kParameterNotInPackageMessage; -static absl::string_view kLocalParamMessage = kLocalParamAllowPackageMessage; - -static absl::string_view kAutoFixReplaceParameterWithLocalparam = +static constexpr absl::string_view kAutoFixReplaceParameterWithLocalparam = "Replace 'parameter' with 'localparam'"; -static absl::string_view kAutoFixReplaceLocalparamWithParameter = +static constexpr absl::string_view kAutoFixReplaceLocalparamWithParameter = "Replace 'localparam' with 'parameter'"; const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { @@ -89,6 +86,19 @@ const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { return d; } +ProperParameterDeclarationRule::ProperParameterDeclarationRule() { + ChooseMessagesForConfiguration(); +} + +void ProperParameterDeclarationRule::ChooseMessagesForConfiguration() { + // Message is slightly different depending on configuration + parameter_message_ = package_allow_parameter_ ? kParameterAllowPackageMessage + : kParameterNotInPackageMessage; + local_parameter_message_ = package_allow_localparam_ + ? kLocalParamAllowPackageMessage + : kLocalParamNotInPackageMessage; +} + absl::Status ProperParameterDeclarationRule::Configure( absl::string_view configuration) { using verible::config::SetBool; @@ -99,12 +109,7 @@ absl::Status ProperParameterDeclarationRule::Configure( {"package_allow_localparam", SetBool(&package_allow_localparam_)}, }); - // Change the message slightly - kParameterMessage = package_allow_parameter_ ? kParameterAllowPackageMessage - : kParameterNotInPackageMessage; - kLocalParamMessage = package_allow_localparam_ - ? kLocalParamAllowPackageMessage - : kLocalParamNotInPackageMessage; + ChooseMessagesForConfiguration(); return status; } @@ -124,7 +129,7 @@ void ProperParameterDeclarationRule::AddParameterViolation( AutoFix autofix = AutoFix(kAutoFixReplaceParameterWithLocalparam, {*token, "localparam"}); violations_.insert( - LintViolation(*token, kParameterMessage, context, {autofix})); + LintViolation(*token, parameter_message_, context, {autofix})); } void ProperParameterDeclarationRule::AddLocalparamViolation( @@ -138,7 +143,7 @@ void ProperParameterDeclarationRule::AddLocalparamViolation( AutoFix autofix = AutoFix(kAutoFixReplaceLocalparamWithParameter, {*token, "parameter"}); violations_.insert( - LintViolation(*token, kLocalParamMessage, context, {autofix})); + LintViolation(*token, local_parameter_message_, context, {autofix})); } // TODO(kathuriac): Also check the 'interface' and 'program' constructs. diff --git a/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.h b/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.h index 810bb1830..e115a168d 100644 --- a/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.h +++ b/verible/verilog/analysis/checkers/proper-parameter-declaration-rule.h @@ -37,6 +37,8 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { static const LintRuleDescriptor &GetDescriptor(); + ProperParameterDeclarationRule(); + void AddParameterViolation(const verible::Symbol &symbol, const verible::SyntaxTreeContext &context); @@ -51,10 +53,15 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { absl::Status Configure(absl::string_view configuration) final; private: + void ChooseMessagesForConfiguration(); + std::set violations_; bool package_allow_parameter_ = false; bool package_allow_localparam_ = true; + + absl::string_view parameter_message_; + absl::string_view local_parameter_message_; }; } // namespace analysis From a9fd79189b1f57b46b2234b567de25638ef63547 Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sat, 23 Nov 2024 11:33:40 -0800 Subject: [PATCH 3/5] Make common lsp-libraries, useful to build a language server, public. This makes it easier to integrate in other projects that would like to use these without having to mess with the visibility. --- verible/common/lsp/BUILD | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/verible/common/lsp/BUILD b/verible/common/lsp/BUILD index 4cd1a67fc..69f93de1f 100644 --- a/verible/common/lsp/BUILD +++ b/verible/common/lsp/BUILD @@ -3,6 +3,9 @@ # LSP is using JSON-PRC [2] for the RPC protocol, the transport layer chunks # messages with header+body blocks, similar to HTTP. # +# All libraries needed to build a language server have the public visibility +# as they are very useful in itself to build language servers in other projects. +# # [1]: https://microsoft.github.io/language-server-protocol/specification # [2]: https://www.jsonrpc.org/specification @@ -20,6 +23,7 @@ cc_library( name = "message-stream-splitter", srcs = ["message-stream-splitter.cc"], hdrs = ["message-stream-splitter.h"], + visibility = ["//visibility:public"], deps = [ "//verible/common/util:status-macros", "@com_google_absl//absl/status", @@ -50,6 +54,7 @@ cc_library( "//conditions:default": ["-fexceptions"], }), features = ["-use_header_modules"], # precompiled headers incompatible with -fexceptions. + visibility = ["//visibility:public"], deps = [ "//verible/common/util:logging", "@com_google_absl//absl/strings:string_view", @@ -85,17 +90,20 @@ genrule( cc_library( name = "lsp-protocol", hdrs = ["lsp-protocol.h"], + visibility = ["//visibility:public"], deps = ["@jsonhpp//:singleheader-json"], ) cc_library( name = "lsp-protocol-enums", hdrs = ["lsp-protocol-enums.h"], + visibility = ["//visibility:public"], ) cc_library( name = "lsp-protocol-operators", hdrs = ["lsp-protocol-operators.h"], + visibility = ["//visibility:public"], deps = [":lsp-protocol"], ) @@ -114,6 +122,7 @@ cc_library( name = "lsp-text-buffer", srcs = ["lsp-text-buffer.cc"], hdrs = ["lsp-text-buffer.h"], + visibility = ["//visibility:public"], deps = [ ":json-rpc-dispatcher", ":lsp-protocol", @@ -142,6 +151,7 @@ cc_library( name = "lsp-file-utils", srcs = ["lsp-file-utils.cc"], hdrs = ["lsp-file-utils.h"], + visibility = ["//visibility:public"], deps = [ "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", From 74eabb6e10ae521cd6a537bc587e8c74d17eda88 Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sat, 23 Nov 2024 12:01:24 -0800 Subject: [PATCH 4/5] Simplify number parsing by using absl::SimpleAtoi() --- .../analysis/checkers/dff-name-style-rule.cc | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/verible/verilog/analysis/checkers/dff-name-style-rule.cc b/verible/verilog/analysis/checkers/dff-name-style-rule.cc index 43ac52923..1211f36c8 100644 --- a/verible/verilog/analysis/checkers/dff-name-style-rule.cc +++ b/verible/verilog/analysis/checkers/dff-name-style-rule.cc @@ -16,19 +16,18 @@ #include #include -#include #include #include #include #include #include -#include #include #include #include "absl/status/status.h" #include "absl/strings/ascii.h" #include "absl/strings/match.h" +#include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" @@ -388,16 +387,9 @@ DffNameStyleRule::ExtractPipelineStage(absl::string_view id) { if (num_digits == 0 || num_digits == id.size()) return {id, {}}; // Extract the integer value for the pipeline stage + const absl::string_view pipe_stage_str = id.substr(id.size() - num_digits); uint64_t pipe_stage; - std::from_chars_result result = std::from_chars( - id.cbegin() + id.size() - num_digits, id.cend(), pipe_stage); - - // https://en.cppreference.com/w/cpp/utility/from_chars - // Check whether: - // - There are errors parsing the string - // - There are non-numeric characters inside the range (shouldn't!) - // - The pipeline stage number is in the valid range - if (result.ec != std::errc() || result.ptr != id.end() || + if (!absl::SimpleAtoi(pipe_stage_str, &pipe_stage) || pipe_stage < kFirstValidPipeStage) { return {id, {}}; } From d4edfb88804db7566a83796b5d3d0f5e2e916a9f Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sat, 23 Nov 2024 12:10:04 -0800 Subject: [PATCH 5/5] CI: fix path to language server subdir. --- .github/workflows/verible-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml index cfb9bdf08..6da773980 100644 --- a/.github/workflows/verible-ci.yml +++ b/.github/workflows/verible-ci.yml @@ -550,7 +550,7 @@ jobs: env: VSC_MARKETPLACE_TOKEN: ${{ secrets.VSC_MARKETPLACE_TOKEN }} run: | - cd verilog/tools/ls/vscode + cd verible/verilog/tools/ls/vscode npm install # install vsce globally npm install -g @vscode/vsce