A few clang-tidy findings addressed

.clang-tidy

@@ -49,6 +49,7 @@ Checks: >
   -readability-isolate-declaration,
   -readability-magic-numbers,
   -readability-make-member-function-const,
+  -readability-math-missing-parentheses,
   -readability-named-parameter,
   -readability-qualified-auto,
   -readability-redundant-access-specifiers,

verible/common/formatting/align.cc

@@ -1083,7 +1083,7 @@ static int MaxOfPositives2D(const std::vector<std::vector<int>> &values) {
   for (const auto &row : values) {
     for (const int delta : row) {
       // Only accumulate positive values.
-      if (delta > result) result = delta;
+      result = std::max(delta, result);
     }
   }
   return result;

verible/common/formatting/layout-optimizer-internal.h

@@ -617,7 +617,7 @@ class LayoutFunctionFactory {
   //
   // Iterator: iterator type that dereferences to LayoutFunction.
   template <class Iterator>
-  LayoutFunction Choice(const Iterator begin, const Iterator end) const {
+  LayoutFunction Choice(const Iterator &begin, const Iterator &end) const {
     static_assert(IsIteratorDereferencingTo<Iterator, LayoutFunction>,
                   "Iterator's value type must be LayoutFunction.");
     const auto lfs = make_container_range(begin, end);

verible/common/formatting/layout-optimizer.cc

@@ -402,7 +402,7 @@ LayoutFunction LayoutFunctionFactory::Stack(
         if ((segment_it + 1).IsEnd()) continue;
         const int column = (segment_it + 1)->column;
         CHECK_GE(column, current_column);
-        if (column < next_column) next_column = column;
+        next_column = std::min(column, next_column);
       }
       current_column = next_column;
     }
@@ -434,7 +434,7 @@ LayoutFunction LayoutFunctionFactory::Choice(
 
       const int column =
           (segment_it + 1).IsEnd() ? kInfinity : segment_it[1].column;
-      if (column < next_knot) next_knot = column;
+      next_knot = std::min(column, next_knot);
     }
 
     do {

verible/common/lsp/lsp-text-buffer.cc

@@ -14,6 +14,7 @@
 
 #include "verible/common/lsp/lsp-text-buffer.h"
 
+#include <algorithm>
 #include <numeric>
 #include <string>
 #include <string_view>
@@ -86,7 +87,7 @@ bool EditTextBuffer::LineEdit(const TextDocumentContentChangeEvent &c,
   if (!str->empty() && str->back() == '\n') --str_end;
 
   if (c.range.start.character > str_end) return false;
-  if (end_char > str_end) end_char = str_end;
+  end_char = std::min(end_char, str_end);
   if (end_char < c.range.start.character) return false;
 
   document_length_ -= str->length();

verible/common/lsp/message-stream-splitter.cc

@@ -48,12 +48,12 @@ int MessageStreamSplitter::ParseHeaderGetBodyOffset(std::string_view data,
   static constexpr std::string_view kEndHeaderMarker = "\r\n\r\n";
   static constexpr std::string_view kContentLengthHeader = "Content-Length: ";
 
-  int header_marker_len = kEndHeaderMarker.length();
-  auto end_of_header = data.find(kEndHeaderMarker);
+  const size_t header_marker_len = kEndHeaderMarker.length();
+  const size_t end_of_header = data.find(kEndHeaderMarker);
   if (end_of_header == std::string_view::npos) return kIncompleteHeader;
 
   // Very dirty search for header - we don't check if starts with line.
-  const std::string_view header_content(data.data(), end_of_header);
+  const std::string_view header_content = data.substr(0, end_of_header);
   auto found_ContentLength_header = header_content.find(kContentLengthHeader);
   if (found_ContentLength_header == std::string_view::npos) {
     return kGarbledHeader;
@@ -80,8 +80,8 @@ absl::Status MessageStreamSplitter::ProcessContainedMessages(
     int body_size = 0;
     const int body_offset = ParseHeaderGetBodyOffset(*data, &body_size);
     if (body_offset == kGarbledHeader) {
-      std::string_view limited_view(
-          data->data(), std::min(data->size(), static_cast<size_t>(256)));
+      const std::string_view limited_view =
+          data->substr(0, std::min(data->size(), static_cast<size_t>(256)));
       return absl::InvalidArgumentError(
           absl::StrCat("No `Content-Length:` header. '",
                        absl::CEscape(limited_view), "...'"));
@@ -93,8 +93,8 @@ absl::Status MessageStreamSplitter::ProcessContainedMessages(
       return absl::OkStatus();  // Only insufficient partial buffer available.
     }
 
-    std::string_view header(data->data(), body_offset);
-    std::string_view body(data->data() + body_offset, body_size);
+    const std::string_view header = data->substr(0, body_offset);
+    const std::string_view body = data->substr(body_offset, body_size);
     message_processor_(header, body);
 
     stats_largest_body_ = std::max(stats_largest_body_, body.size());

verible/common/text/macro-definition.cc

@@ -104,7 +104,7 @@ const TokenInfo &MacroDefinition::SubstituteText(
     }
   }
   // Didn't match enum type nor find map entry, so don't substitute.
-  return token_info;
+  return token_info;  // NOLINT(bugprone-return-const-ref-from-parameter)
 }
 
 }  // namespace verible

verible/common/text/tree-utils.h

@@ -54,11 +54,9 @@ SyntaxTreeNode &SymbolCastToNode(Symbol &);  // NOLINT
 // The following no-op overloads allow SymbolCastToNode() to work with zero
 // overhead when the argument type is statically known to be the same.
 inline const SyntaxTreeNode &SymbolCastToNode(const SyntaxTreeNode &node) {
-  return node;
-}
-inline SyntaxTreeNode &SymbolCastToNode(SyntaxTreeNode &node) {  // NOLINT
-  return node;
+  return node;  // NOLINT(bugprone-return-const-ref-from-parameter)
 }
+inline SyntaxTreeNode &SymbolCastToNode(SyntaxTreeNode &node) { return node; }
 
 // Returns a SyntaxTreeLeaf down_casted from a Symbol.
 const SyntaxTreeLeaf &SymbolCastToLeaf(const Symbol &);

verible/common/util/container-proxy_test.cc

@@ -113,7 +113,7 @@ struct TestTrace {
            std::equal(values.begin(), values.end(), other.values.begin());
   }
 
-  friend std::ostream &operator<<(std::ostream &s, TestTrace obj) {
+  friend std::ostream &operator<<(std::ostream &s, const TestTrace &obj) {
     switch (obj.triggered_method) {
       case ContainerProxyEvent::kUnknown:
         s << "UNKNOWN";

verible/common/util/container-util.h

@@ -43,7 +43,8 @@ struct ValueTypeFromKeyType<KeyType, std::pair<const KeyType, MappedType>> {
 template <class KeyType>
 struct ValueTypeFromKeyType<KeyType, KeyType> {
   const KeyType &operator()(const KeyType &k) const {
-    return k;  // just forward the key
+    // Just forward the key
+    return k;  // NOLINT(bugprone-return-const-ref-from-parameter)
   }
 };
 }  // namespace internal

verible/common/util/file-util.cc

@@ -179,10 +179,10 @@ absl::StatusOr<std::string> GetContentAsString(std::string_view filename) {
   }
   char buffer[4096];
   int bytes_read;
-  do {
-    bytes_read = fread(buffer, 1, sizeof(buffer), stream);
+  while (!ferror(stream) && !feof(stream) &&
+         (bytes_read = fread(buffer, 1, sizeof(buffer), stream)) >= 0) {
     content.append(buffer, bytes_read);
-  } while (bytes_read > 0);
+  }
   fclose(stream);
 
   return content;

verible/common/util/forward.h

@@ -31,7 +31,9 @@ struct ForwardReferenceElseConstruct {
   // Intentionally restricted to const reference to avoid the surprise
   // of modifying a temporary reference.
   // See also the two-parameter overload.
-  const T &operator()(const T &t) const { return t; }
+  const T &operator()(const T &t) const {
+    return t;  // NOLINT(bugprone-return-const-ref-from-parameter)
+  }
 
   // (overload) Constructs a temporary rvalue, when types are different.
   // This works with explicit-only constructors and implicit constructors.

verible/verilog/analysis/checkers/constraint-name-style-rule.cc

@@ -51,7 +51,7 @@ const LintRuleDescriptor &ConstraintNameStyleRule::GetDescriptor() {
       .desc =
           "Check that constraint names follow the required name style "
           "specified by a regular expression.",
-      .param = {{"pattern", kSuffix.data()}},
+      .param = {{"pattern", std::string(kSuffix)}},
   };
   return d;
 }

verible/verilog/analysis/verilog-linter-configuration.cc

@@ -281,7 +281,8 @@ void LinterConfiguration::GetRuleBundle(RuleBundle *rule_bundle) const {
 template <typename T>
 static absl::StatusOr<std::vector<std::unique_ptr<T>>> CreateRules(
     const std::map<analysis::LintRuleId, RuleSetting> &config,
-    std::function<std::unique_ptr<T>(const analysis::LintRuleId &)> factory) {
+    const std::function<std::unique_ptr<T>(const analysis::LintRuleId &)>
+        &factory) {
   std::vector<std::unique_ptr<T>> rule_instances;
   for (const auto &rule_pair : config) {
     const RuleSetting &setting = rule_pair.second;

verible/verilog/formatting/token-annotator.cc

@@ -240,9 +240,7 @@ static WithReason<int> SpacesRequiredBetween(
       }
 
       int spaces = right.OriginalLeadingSpaces().length();
-      if (spaces > 1) {
-        spaces = 1;
-      }
+      spaces = std::min(spaces, 1);
       return {spaces, "Limit <= 1 space before binary operator inside []."};
     }
     if (left.format_token_enum == FormatTokenType::binary_operator &&

verible/verilog/tools/ls/autoexpand.cc

@@ -548,10 +548,8 @@ Dimension MaxDimension(const size_t first, const size_t second) {
 template <>
 Dimension MaxDimension(const DimensionRange first,
                        const DimensionRange second) {
-  int64_t max = std::max(std::max(first.msb, first.lsb),
-                         std::max(second.msb, second.lsb));
-  int64_t min = std::min(std::min(first.msb, first.lsb),
-                         std::min(second.msb, second.lsb));
+  int64_t max = std::max({first.msb, first.lsb, second.msb, second.lsb});
+  int64_t min = std::min({first.msb, first.lsb, second.msb, second.lsb});
   if (first.msb >= first.lsb) {
     return DimensionRange{.msb = max, .lsb = min};
   }