From 0dabd1b803f14fd77e525868a9c3bf276b414ae4 Mon Sep 17 00:00:00 2001 From: David Fang Date: Tue, 16 Jun 2020 16:11:48 -0700 Subject: [PATCH] Define LineNumberSet as its own class (subclass of IntervalSet). Combine near-equivalent typedefs in the codebase. PiperOrigin-RevId: 316777543 --- common/analysis/BUILD | 2 ++ common/analysis/lint_waiver.cc | 4 ++-- common/analysis/lint_waiver.h | 11 +++++------ common/strings/position.h | 20 ++++++++++++++++++++ verilog/analysis/verilog_linter.cc | 5 +++-- verilog/formatting/BUILD | 2 ++ verilog/formatting/comment_controls.cc | 2 +- verilog/formatting/comment_controls.h | 8 ++------ verilog/formatting/comment_controls_test.cc | 1 + verilog/formatting/formatter.cc | 1 + verilog/formatting/formatter.h | 4 ++-- verilog/formatting/formatter_test.cc | 2 ++ verilog/formatting/formatter_tuning_test.cc | 3 ++- verilog/tools/formatter/BUILD | 1 + verilog/tools/formatter/verilog_format.cc | 6 ++++-- 15 files changed, 50 insertions(+), 22 deletions(-) diff --git a/common/analysis/BUILD b/common/analysis/BUILD index 077ed0f8c..be60d10c4 100644 --- a/common/analysis/BUILD +++ b/common/analysis/BUILD @@ -66,6 +66,7 @@ cc_library( "//bazel:flex", "//common/lexer:flex_lexer_adapter", "//common/lexer:token_stream_adapter", + "//common/strings:position", "//common/text:text_structure", "//common/text:token_info", "//common/text:token_stream_view", @@ -86,6 +87,7 @@ cc_library( ":command_file_lexer", "//common/strings:comment_utils", "//common/strings:line_column_map", + "//common/strings:position", "//common/text:text_structure", "//common/text:token_info", "//common/text:token_stream_view", diff --git a/common/analysis/lint_waiver.cc b/common/analysis/lint_waiver.cc index f7098b673..bc5f951d5 100644 --- a/common/analysis/lint_waiver.cc +++ b/common/analysis/lint_waiver.cc @@ -46,7 +46,7 @@ void LintWaiver::WaiveOneLine(absl::string_view rule_name, size_t line_number) { void LintWaiver::WaiveLineRange(absl::string_view rule_name, size_t line_begin, size_t line_end) { - LineSet& line_set = waiver_map_[rule_name]; + LineNumberSet& line_set = waiver_map_[rule_name]; line_set.Add({line_begin, line_end}); } @@ -78,7 +78,7 @@ void LintWaiver::RegexToLines(absl::string_view contents, bool LintWaiver::RuleIsWaivedOnLine(absl::string_view rule_name, size_t line_number) const { const auto* line_set = verible::container::FindOrNull(waiver_map_, rule_name); - return line_set != nullptr && LineSetContains(*line_set, line_number); + return line_set != nullptr && LineNumberSetContains(*line_set, line_number); } bool LintWaiver::Empty() const { diff --git a/common/analysis/lint_waiver.h b/common/analysis/lint_waiver.h index 318cc2644..c052fb5a9 100644 --- a/common/analysis/lint_waiver.h +++ b/common/analysis/lint_waiver.h @@ -22,6 +22,7 @@ #include #include "absl/strings/string_view.h" +#include "common/strings/position.h" #include "common/text/text_structure.h" #include "common/text/token_stream_view.h" #include "common/util/container_util.h" @@ -32,9 +33,6 @@ namespace verible { // LintWaiver maintains a set of line ranges per lint rule that should be // exempt from each rule. class LintWaiver { - // Compact set of line numbers. - // TODO(b/156991337): combine with other definition of LineNumberSet - using LineSet = IntervalSet; using RegexVector = std::vector; public: @@ -73,12 +71,13 @@ class LintWaiver { // TODO(hzeller): The following methods break abstraction and are only // for performance. Reconsider if this is worth it. - const LineSet* LookupLineSet(absl::string_view rule_name) const { + const LineNumberSet* LookupLineNumberSet(absl::string_view rule_name) const { return verible::container::FindOrNull(waiver_map_, rule_name); } // Test if a particular line is included in the set. - static bool LineSetContains(const LineSet& line_set, size_t line) { + static bool LineNumberSetContains(const LineNumberSet& line_set, + size_t line) { return line_set.Contains(line); } @@ -87,7 +86,7 @@ class LintWaiver { // string_view because the static strings for each lint rule class exist, // and will outlive all LintWaiver objects. This applies to both waiver_map_ // and waiver_re_map_. - std::map waiver_map_; + std::map waiver_map_; std::map waiver_re_map_; std::map regex_cache_; diff --git a/common/strings/position.h b/common/strings/position.h index 72372e832..5989e7724 100644 --- a/common/strings/position.h +++ b/common/strings/position.h @@ -15,6 +15,8 @@ #ifndef VERIBLE_COMMON_STRINGS_POSITION_H_ #define VERIBLE_COMMON_STRINGS_POSITION_H_ +#include + #include "absl/strings/string_view.h" #include "common/util/interval_set.h" @@ -45,6 +47,24 @@ class ByteOffsetSet : public verible::IntervalSet { : impl_type(ranges) {} }; +// Collection of ranges of line numbers. +// Intentionally defining this as its own class instead of a typedef to +// avoid potential confusion with other IntervalSet. +// Mismatches will be caught as type errors. +class LineNumberSet : public verible::IntervalSet { + typedef verible::IntervalSet impl_type; + + public: + LineNumberSet() = default; + + explicit LineNumberSet(const impl_type& iset) : impl_type(iset) {} + + // This constructor can initialize from a sequence of pairs, e.g. + // LineNumberSet s{{0,1}, {4,7}, {8,10}}; + LineNumberSet(std::initializer_list> ranges) + : impl_type(ranges) {} +}; + } // namespace verible #endif // VERIBLE_COMMON_STRINGS_POSITION_H_ diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index c8f140ca7..b9eff0d4b 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -199,7 +199,8 @@ static void AppendLintRuleStatuses( std::vector* cumulative_statuses) { for (const auto& status : new_statuses) { cumulative_statuses->push_back(status); - const auto* waived_lines = waivers.LookupLineSet(status.lint_rule_name); + const auto* waived_lines = + waivers.LookupLineNumberSet(status.lint_rule_name); if (waived_lines) { cumulative_statuses->back().WaiveViolations( [&](const verible::LintViolation& violation) { @@ -208,7 +209,7 @@ static void AppendLintRuleStatuses( const size_t line = line_map(offset).line; // Check that line number against the set of waived lines. const bool waived = - LintWaiver::LineSetContains(*waived_lines, line); + LintWaiver::LineNumberSetContains(*waived_lines, line); VLOG(2) << "Violation of " << status.lint_rule_name << " rule on line " << line + 1 << (waived ? " is waived." : " is not waived."); diff --git a/verilog/formatting/BUILD b/verilog/formatting/BUILD index 8e30fcc59..1c586d402 100644 --- a/verilog/formatting/BUILD +++ b/verilog/formatting/BUILD @@ -160,6 +160,7 @@ cc_test( deps = [ ":format_style", ":formatter", + "//common/strings:position", "//common/text:text_structure", "//common/util:logging", "//verilog/analysis:verilog_analyzer", @@ -175,6 +176,7 @@ cc_test( deps = [ ":format_style", ":formatter", + "//common/strings:position", "//common/util:logging", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", diff --git a/verilog/formatting/comment_controls.cc b/verilog/formatting/comment_controls.cc index aa932bc77..046295c45 100644 --- a/verilog/formatting/comment_controls.cc +++ b/verilog/formatting/comment_controls.cc @@ -88,7 +88,7 @@ ByteOffsetSet DisableFormattingRanges(absl::string_view text, } ByteOffsetSet EnabledLinesToDisabledByteRanges( - const LineNumberSet& line_numbers, + const verible::LineNumberSet& line_numbers, const verible::LineColumnMap& line_column_map) { // Interpret empty line numbers as enabling all lines for formatting. if (line_numbers.empty()) return ByteOffsetSet(); diff --git a/verilog/formatting/comment_controls.h b/verilog/formatting/comment_controls.h index 91838ada4..a575cb477 100644 --- a/verilog/formatting/comment_controls.h +++ b/verilog/formatting/comment_controls.h @@ -17,17 +17,13 @@ #include "absl/strings/string_view.h" #include "common/strings/line_column_map.h" -#include "common/strings/position.h" // for ByteOffsetSet +#include "common/strings/position.h" // for ByteOffsetSet, LineNumberSet #include "common/text/token_stream_view.h" #include "common/util/interval_set.h" namespace verilog { namespace formatter { -// Collection of line numbers, 1-based. -// TODO(b/156991337): combine with other definition for LineSet -using LineNumberSet = verible::IntervalSet; - // Returns a representation of byte offsets where true (membership) means // formatting is disabled. verible::ByteOffsetSet DisableFormattingRanges( @@ -38,7 +34,7 @@ verible::ByteOffsetSet DisableFormattingRanges( // Translates line numbers into a set of byte ranges to disable formatting. verible::ByteOffsetSet EnabledLinesToDisabledByteRanges( - const LineNumberSet& line_numbers, + const verible::LineNumberSet& line_numbers, const verible::LineColumnMap& line_column_map); // Formats space between tokens while honoring formatting-disabled ranges. diff --git a/verilog/formatting/comment_controls_test.cc b/verilog/formatting/comment_controls_test.cc index 5d195467e..5a967872c 100644 --- a/verilog/formatting/comment_controls_test.cc +++ b/verilog/formatting/comment_controls_test.cc @@ -31,6 +31,7 @@ using ::testing::ElementsAre; using verible::ByteOffsetSet; using verible::ExpectedTokenInfo; using verible::LineColumnMap; +using verible::LineNumberSet; using verible::TokenInfoTestData; TEST(DisableFormattingRangesTest, EmptyFile) { diff --git a/verilog/formatting/formatter.cc b/verilog/formatting/formatter.cc index bac2506ab..40830a2b6 100644 --- a/verilog/formatting/formatter.cc +++ b/verilog/formatting/formatter.cc @@ -56,6 +56,7 @@ using absl::StatusCode; using verible::ByteOffsetSet; using verible::ExpandableTreeView; +using verible::LineNumberSet; using verible::MutableFormatTokenRange; using verible::PartitionPolicyEnum; using verible::PreFormatToken; diff --git a/verilog/formatting/formatter.h b/verilog/formatting/formatter.h index fbe367676..5d6fb8687 100644 --- a/verilog/formatting/formatter.h +++ b/verilog/formatting/formatter.h @@ -19,7 +19,7 @@ #include #include "absl/status/status.h" -#include "verilog/formatting/comment_controls.h" +#include "common/strings/position.h" #include "verilog/formatting/format_style.h" namespace verilog { @@ -71,7 +71,7 @@ struct ExecutionControl { absl::Status FormatVerilog(absl::string_view text, absl::string_view filename, const FormatStyle& style, std::ostream& formatted_stream, - const LineNumberSet& lines = {}, + const verible::LineNumberSet& lines = {}, const ExecutionControl& control = {}); } // namespace formatter diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index 79f2bd795..fc18fd036 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -31,6 +31,7 @@ #include "absl/status/status.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" +#include "common/strings/position.h" #include "common/text/text_structure.h" #include "common/util/logging.h" #include "verilog/analysis/verilog_analyzer.h" @@ -53,6 +54,7 @@ absl::Status VerifyFormatting(const verible::TextStructureView& text_structure, namespace { using absl::StatusCode; +using verible::LineNumberSet; // Tests that clean output passes. TEST(VerifyFormattingTest, NoError) { diff --git a/verilog/formatting/formatter_tuning_test.cc b/verilog/formatting/formatter_tuning_test.cc index bd5b465ad..ede314129 100644 --- a/verilog/formatting/formatter_tuning_test.cc +++ b/verilog/formatting/formatter_tuning_test.cc @@ -23,6 +23,7 @@ #include "gtest/gtest.h" #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "common/strings/position.h" #include "common/util/logging.h" #include "verilog/formatting/format_style.h" @@ -41,7 +42,7 @@ struct FormatterTestCase { absl::string_view expected; }; -static const LineNumberSet kEnableAllLines; +static const verible::LineNumberSet kEnableAllLines; // Tests in this file are intended to be sensitive to wrapping penalty tuning. // These test cases should be kept short, small enough to be directed diff --git a/verilog/tools/formatter/BUILD b/verilog/tools/formatter/BUILD index c08134a4a..d2060b7ea 100644 --- a/verilog/tools/formatter/BUILD +++ b/verilog/tools/formatter/BUILD @@ -8,6 +8,7 @@ cc_binary( srcs = ["verilog_format.cc"], visibility = ["//visibility:public"], # for verilog_style_lint.bzl deps = [ + "//common/strings:position", "//common/util:file_util", "//common/util:init_command_line", "//common/util:interval_set", diff --git a/verilog/tools/formatter/verilog_format.cc b/verilog/tools/formatter/verilog_format.cc index a55f780e6..226e28e8f 100644 --- a/verilog/tools/formatter/verilog_format.cc +++ b/verilog/tools/formatter/verilog_format.cc @@ -36,6 +36,7 @@ #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "common/strings/position.h" #include "common/util/file_util.h" #include "common/util/init_command_line.h" #include "common/util/interval_set.h" @@ -44,6 +45,7 @@ #include "verilog/formatting/formatter.h" using absl::StatusCode; +using verible::LineNumberSet; using verilog::formatter::ExecutionControl; using verilog::formatter::FormatStyle; using verilog::formatter::FormatVerilog; @@ -130,7 +132,7 @@ std::ostream& FileMsg(absl::string_view filename) { } bool formatOneFile(absl::string_view filename, - const verilog::formatter::LineNumberSet& lines_to_format) { + const LineNumberSet& lines_to_format) { const bool inplace = absl::GetFlag(FLAGS_inplace); const bool is_stdin = filename == "-"; const auto& stdin_name = absl::GetFlag(FLAGS_stdin_name); @@ -245,7 +247,7 @@ int main(int argc, char** argv) { } // Parse LineRanges into a line set, to validate the --lines flag(s) - verilog::formatter::LineNumberSet lines_to_format; + LineNumberSet lines_to_format; if (!verible::ParseInclusiveRanges( &lines_to_format, LineRanges::values.begin(), LineRanges::values.end(), &std::cerr, '-')) {