Merge pull request chipsalliance#2293 from hzeller/feature-20241123-l…

…ittle-cleanups Fix non-const globals; make common lsp libraries public visibility
hzeller · Nov 23, 2024 · fd287ef · fd287ef
2 parents 7850f77 + d4edfb8
commit fd287ef
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 34 deletions.
diff --git 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

diff --git 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
+  '';
 }
diff --git 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/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",

diff --git 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 <algorithm>
 #include <cctype>
-#include <charconv>
 #include <cstdint>
 #include <iterator>
 #include <optional>
 #include <set>
 #include <string>
-#include <system_error>
 #include <utility>
 #include <vector>
 
 #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, {}};
   }

diff --git 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<re2::RE2>(upper_snake_case_regex, re2::RE2::Quiet)),
+          std::make_unique<re2::RE2>(kUpperSnakeCaseRegex, re2::RE2::Quiet)),
       style_lower_snake_case_regex_(
-          std::make_unique<re2::RE2>(lower_snake_case_regex, re2::RE2::Quiet)),
-      style_upper_snake_case_regex_(std::make_unique<re2::RE2>(
-          upper_snake_case_regex, re2::RE2::Quiet)) {}
+          std::make_unique<re2::RE2>(kLowerSnakeCaseRegex, re2::RE2::Quiet)),
+      style_upper_snake_case_regex_(
+          std::make_unique<re2::RE2>(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
@@ -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
@@ -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<verible::LintViolation> violations_;
 
   bool package_allow_parameter_ = false;
   bool package_allow_localparam_ = true;
+
+  absl::string_view parameter_message_;
+  absl::string_view local_parameter_message_;
 };
 
 }  // namespace analysis