From 5a063c4b377e03ce6cb791ab1a4617cc701db5af Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sun, 16 Feb 2025 10:11:04 -0800 Subject: [PATCH 1/2] Use std::min()/std::max() to improve readability. --- verible/common/formatting/align.cc | 2 +- verible/common/formatting/layout-optimizer.cc | 4 ++-- verible/common/lsp/lsp-text-buffer.cc | 3 ++- verible/verilog/formatting/token-annotator.cc | 4 +--- verible/verilog/tools/ls/autoexpand.cc | 6 ++---- 5 files changed, 8 insertions(+), 11 deletions(-) diff --git a/verible/common/formatting/align.cc b/verible/common/formatting/align.cc index 6f69ef39f..eb1c354d9 100644 --- a/verible/common/formatting/align.cc +++ b/verible/common/formatting/align.cc @@ -1083,7 +1083,7 @@ static int MaxOfPositives2D(const std::vector> &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; diff --git a/verible/common/formatting/layout-optimizer.cc b/verible/common/formatting/layout-optimizer.cc index 94280770f..145968781 100644 --- a/verible/common/formatting/layout-optimizer.cc +++ b/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 { diff --git a/verible/common/lsp/lsp-text-buffer.cc b/verible/common/lsp/lsp-text-buffer.cc index 9cf7b80d4..27e4cf4b4 100644 --- a/verible/common/lsp/lsp-text-buffer.cc +++ b/verible/common/lsp/lsp-text-buffer.cc @@ -14,6 +14,7 @@ #include "verible/common/lsp/lsp-text-buffer.h" +#include #include #include #include @@ -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(); diff --git a/verible/verilog/formatting/token-annotator.cc b/verible/verilog/formatting/token-annotator.cc index 7a4a063fd..7a33214b2 100644 --- a/verible/verilog/formatting/token-annotator.cc +++ b/verible/verilog/formatting/token-annotator.cc @@ -240,9 +240,7 @@ static WithReason 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 && diff --git a/verible/verilog/tools/ls/autoexpand.cc b/verible/verilog/tools/ls/autoexpand.cc index c1ab6b385..c2683e292 100644 --- a/verible/verilog/tools/ls/autoexpand.cc +++ b/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}; } From 430f8d49a26ddddb0bd9d9810be537a75909334c Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Sun, 16 Feb 2025 11:28:52 -0800 Subject: [PATCH 2/2] Address a few clang-tidy findings. --- .clang-tidy | 1 + .../common/formatting/layout-optimizer-internal.h | 2 +- verible/common/lsp/message-stream-splitter.cc | 14 +++++++------- verible/common/text/macro-definition.cc | 2 +- verible/common/text/tree-utils.h | 6 ++---- verible/common/util/container-proxy_test.cc | 2 +- verible/common/util/container-util.h | 3 ++- verible/common/util/file-util.cc | 6 +++--- verible/common/util/forward.h | 4 +++- .../checkers/constraint-name-style-rule.cc | 2 +- .../analysis/verilog-linter-configuration.cc | 3 ++- 11 files changed, 24 insertions(+), 21 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index e21031934..be00837ad 100644 --- a/.clang-tidy +++ b/.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, diff --git a/verible/common/formatting/layout-optimizer-internal.h b/verible/common/formatting/layout-optimizer-internal.h index cd489f7d0..ed59fe7c3 100644 --- a/verible/common/formatting/layout-optimizer-internal.h +++ b/verible/common/formatting/layout-optimizer-internal.h @@ -617,7 +617,7 @@ class LayoutFunctionFactory { // // Iterator: iterator type that dereferences to LayoutFunction. template - LayoutFunction Choice(const Iterator begin, const Iterator end) const { + LayoutFunction Choice(const Iterator &begin, const Iterator &end) const { static_assert(IsIteratorDereferencingTo, "Iterator's value type must be LayoutFunction."); const auto lfs = make_container_range(begin, end); diff --git a/verible/common/lsp/message-stream-splitter.cc b/verible/common/lsp/message-stream-splitter.cc index 3daf86403..a3c07d9cf 100644 --- a/verible/common/lsp/message-stream-splitter.cc +++ b/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(256))); + const std::string_view limited_view = + data->substr(0, std::min(data->size(), static_cast(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()); diff --git a/verible/common/text/macro-definition.cc b/verible/common/text/macro-definition.cc index d1a32aad7..7f795f08b 100644 --- a/verible/common/text/macro-definition.cc +++ b/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 diff --git a/verible/common/text/tree-utils.h b/verible/common/text/tree-utils.h index a69cfaa01..8a18924d5 100644 --- a/verible/common/text/tree-utils.h +++ b/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 &); diff --git a/verible/common/util/container-proxy_test.cc b/verible/common/util/container-proxy_test.cc index b5766fde9..5e8220fd6 100644 --- a/verible/common/util/container-proxy_test.cc +++ b/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"; diff --git a/verible/common/util/container-util.h b/verible/common/util/container-util.h index b27f12c44..85bfa91b1 100644 --- a/verible/common/util/container-util.h +++ b/verible/common/util/container-util.h @@ -43,7 +43,8 @@ struct ValueTypeFromKeyType> { template struct ValueTypeFromKeyType { 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 diff --git a/verible/common/util/file-util.cc b/verible/common/util/file-util.cc index bd94cf651..27a4c6f1e 100644 --- a/verible/common/util/file-util.cc +++ b/verible/common/util/file-util.cc @@ -179,10 +179,10 @@ absl::StatusOr 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; diff --git a/verible/common/util/forward.h b/verible/common/util/forward.h index 977810809..56af1259f 100644 --- a/verible/common/util/forward.h +++ b/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. diff --git a/verible/verilog/analysis/checkers/constraint-name-style-rule.cc b/verible/verilog/analysis/checkers/constraint-name-style-rule.cc index c6be316ff..0c66ab995 100644 --- a/verible/verilog/analysis/checkers/constraint-name-style-rule.cc +++ b/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; } diff --git a/verible/verilog/analysis/verilog-linter-configuration.cc b/verible/verilog/analysis/verilog-linter-configuration.cc index 489059a17..07cbef41f 100644 --- a/verible/verilog/analysis/verilog-linter-configuration.cc +++ b/verible/verilog/analysis/verilog-linter-configuration.cc @@ -281,7 +281,8 @@ void LinterConfiguration::GetRuleBundle(RuleBundle *rule_bundle) const { template static absl::StatusOr>> CreateRules( const std::map &config, - std::function(const analysis::LintRuleId &)> factory) { + const std::function(const analysis::LintRuleId &)> + &factory) { std::vector> rule_instances; for (const auto &rule_pair : config) { const RuleSetting &setting = rule_pair.second;