From 351efd9381496755a194fac143a3a03fba9cb5c6 Mon Sep 17 00:00:00 2001 From: David Fang Date: Fri, 14 Aug 2020 14:09:55 -0700 Subject: [PATCH] Leave long-line wrapped sections of text preserved. Line wrap optimization and penalty tuning are the weakest links of the formatter. Attempt to do-less-harm and leave enabled uncontroversial formatting changes. This still allows indentation at the start of such token ranges to occur, so there is still a risk of line-length overflow. PiperOrigin-RevId: 326726800 --- verilog/formatting/format_style.h | 7 ++++ verilog/formatting/formatter.cc | 36 +++++++++++++----- verilog/formatting/formatter_test.cc | 45 +++++++++++++++++++++++ verilog/formatting/tree_unwrapper.cc | 14 +++---- verilog/tools/formatter/verilog_format.cc | 5 +++ 5 files changed, 91 insertions(+), 16 deletions(-) diff --git a/verilog/formatting/format_style.h b/verilog/formatting/format_style.h index 8ac0c9c1b..1b5d3ac0f 100644 --- a/verilog/formatting/format_style.h +++ b/verilog/formatting/format_style.h @@ -49,6 +49,13 @@ struct FormatStyle : public verible::BasicFormatStyle { // and promote the control for this into an enum: {off, compact, align} bool format_module_instantiations = true; + // At this time line wrap optimization is problematic and risks ruining + // otherwise reasonable code. When set to false, this switch will make the + // formatter give-up and leave code as-is in cases where it would otherwise + // attempt to do line wrap optimization. By doing nothing in those cases, we + // reduce the risk of harming already decent code. + bool try_wrap_long_lines = true; + // TODO(fangism): introduce the following knobs: // // Unless forced by previous line, starting a line with a comma is diff --git a/verilog/formatting/formatter.cc b/verilog/formatting/formatter.cc index eee877c1e..0353cc176 100644 --- a/verilog/formatting/formatter.cc +++ b/verilog/formatting/formatter.cc @@ -269,10 +269,11 @@ Status FormatVerilog(absl::string_view text, absl::string_view filename, // Decided at each node in UnwrappedLine partition tree whether or not // it should be expanded or unexpanded. -static void DeterminePartitionExpansion(partition_node_type* node, - absl::string_view full_text, - const ByteOffsetSet& disabled_ranges, - const FormatStyle& style) { +static void DeterminePartitionExpansion( + partition_node_type* node, + std::vector* preformatted_tokens, + absl::string_view full_text, const ByteOffsetSet& disabled_ranges, + const FormatStyle& style) { auto& node_view = node->Value(); const auto& children = node->Children(); @@ -345,8 +346,21 @@ static void DeterminePartitionExpansion(partition_node_type* node, VLOG(3) << "Fits, un-expanding."; node_view.Unexpand(); } else { - VLOG(3) << "Does not fit, expanding."; - node_view.Expand(); + if (style.try_wrap_long_lines) { + VLOG(3) << "Does not fit, expanding."; + node_view.Expand(); + } else { + // give-up early and preserve original spacing + VLOG(3) << "Does not fit, preserving."; + node_view.Unexpand(); + const auto range = node_view.Value().TokensRange(); + const ByteOffsetSet new_disable_range{ + {range.front().token->left(full_text) + 1, + // +1 allows left indentation to be adjusted + range.back().token->right(full_text)}}; + verible::PreserveSpacesOnDisabledTokenRanges( + preformatted_tokens, new_disable_range, full_text); + } } } } @@ -355,6 +369,7 @@ static void DeterminePartitionExpansion(partition_node_type* node, // Produce a worklist of independently formattable UnwrappedLines. static std::vector MakeUnwrappedLinesWorklist( const TokenPartitionTree& format_tokens_partitions, + std::vector* preformatted_tokens, absl::string_view full_text, const ByteOffsetSet& disabled_ranges, const FormatStyle& style) { // Initialize a tree view that treats partitions as fully-expanded. @@ -365,8 +380,10 @@ static std::vector MakeUnwrappedLinesWorklist( // Post-order traversal: if a child doesn't 'fit' and needs to be expanded, // so must all of its parents (and transitively, ancestors). format_tokens_partition_view.ApplyPostOrder( - [&full_text, &disabled_ranges, &style](partition_node_type& node) { - DeterminePartitionExpansion(&node, full_text, disabled_ranges, style); + [&full_text, &disabled_ranges, &style, + preformatted_tokens](partition_node_type& node) { + DeterminePartitionExpansion(&node, preformatted_tokens, full_text, + disabled_ranges, style); }); // Remove trailing blank lines. @@ -533,7 +550,8 @@ Status Formatter::Format(const ExecutionControl& control) { // Produce sequence of independently operable UnwrappedLines. const auto unwrapped_lines = MakeUnwrappedLinesWorklist( - *format_tokens_partitions, full_text, disabled_ranges_, style_); + *format_tokens_partitions, &unwrapper_data.preformatted_tokens, full_text, + disabled_ranges_, style_); // For each UnwrappedLine: minimize total penalty of wrap/break decisions. // TODO(fangism): This could be parallelized if results are written diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index bf7553a1c..cc3d5f2cb 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -5697,6 +5697,51 @@ TEST(FormatterEndToEndTest, DisableModuleInstantiations) { } } +TEST(FormatterEndToEndTest, DisableTryWrapLongLines) { + const std::initializer_list kTestCases = { + {"", ""}, + {"\n", "\n"}, + {"\n\n", "\n\n"}, + {"module m ;\t\n" + " endmodule\n", + "module m;\n" + "endmodule\n"}, + {"module m( ) ;\n" + " endmodule\n", + "module m ();\n" + "endmodule\n"}, + {"module m( ) ;\n" + "initial assign a = b;\n" + " endmodule\n", + "module m ();\n" + " initial assign a = b;\n" + "endmodule\n"}, + {"module m( ) ;\n" + "initial assign a = {never +gonna +give +you +up,\n" // over 40 columns, + // give up + "never + gonna +Let +you +down};\n" + " endmodule\n", + "module m ();\n" + " initial assign a = {never +gonna +give +you +up,\n" + "never + gonna +Let +you +down};\n" + "endmodule\n"}, + }; + FormatStyle style; + style.column_limit = 40; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + style.try_wrap_long_lines = false; + for (const auto& test_case : kTestCases) { + VLOG(1) << "code-to-format:\n" << test_case.input << ""; + std::ostringstream stream; + const auto status = + FormatVerilog(test_case.input, "", style, stream); + // Require these test cases to be valid. + EXPECT_OK(status) << status.message(); + EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input; + } +} + struct SelectLinesTestCase { absl::string_view input; LineNumberSet lines; // explicit set of lines to enable formatting diff --git a/verilog/formatting/tree_unwrapper.cc b/verilog/formatting/tree_unwrapper.cc index ed9004be8..71e7e1478 100644 --- a/verilog/formatting/tree_unwrapper.cc +++ b/verilog/formatting/tree_unwrapper.cc @@ -665,18 +665,11 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions( case NodeEnum::kGenerateRegion: case NodeEnum::kCaseGenerateConstruct: case NodeEnum::kLoopGenerateConstruct: - case NodeEnum::kClassDeclaration: case NodeEnum::kClassConstructor: case NodeEnum::kPackageImportDeclaration: // TODO(fangism): case NodeEnum::kDPIExportItem: case NodeEnum::kPreprocessorInclude: case NodeEnum::kPreprocessorUndef: - case NodeEnum::kModuleDeclaration: - case NodeEnum::kProgramDeclaration: - case NodeEnum::kPackageDeclaration: - case NodeEnum::kInterfaceDeclaration: - case NodeEnum::kFunctionDeclaration: - case NodeEnum::kTaskDeclaration: case NodeEnum::kTFPortDeclaration: case NodeEnum::kTypeDeclaration: case NodeEnum::kForwardDeclaration: @@ -752,6 +745,13 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions( // The following cases will always expand into their constituent // partitions: + case NodeEnum::kModuleDeclaration: + case NodeEnum::kProgramDeclaration: + case NodeEnum::kPackageDeclaration: + case NodeEnum::kInterfaceDeclaration: + case NodeEnum::kFunctionDeclaration: + case NodeEnum::kTaskDeclaration: + case NodeEnum::kClassDeclaration: case NodeEnum::kClassHeader: case NodeEnum::kBegin: case NodeEnum::kEnd: diff --git a/verilog/tools/formatter/verilog_format.cc b/verilog/tools/formatter/verilog_format.cc index 14df2c5e6..f0f94ba93 100644 --- a/verilog/tools/formatter/verilog_format.cc +++ b/verilog/tools/formatter/verilog_format.cc @@ -124,6 +124,10 @@ ABSL_FLAG(bool, format_module_port_declarations, true, ABSL_FLAG(bool, format_module_instantiations, true, "If true, format module instantiations (data declarations), " "else leave them unformatted. This is a short-term workaround."); +ABSL_FLAG(bool, try_wrap_long_lines, false, + "If true, let the formatter attempt to optimize line wrapping " + "decisions where wrapping is needed, else leave them unformatted. " + "This is a short-term measure to reduce risk-of-harm."); static std::ostream& FileMsg(absl::string_view filename) { std::cerr << filename << ": "; @@ -181,6 +185,7 @@ static bool formatOneFile(absl::string_view filename, absl::GetFlag(FLAGS_format_module_port_declarations); format_style.format_module_instantiations = absl::GetFlag(FLAGS_format_module_instantiations); + format_style.try_wrap_long_lines = absl::GetFlag(FLAGS_try_wrap_long_lines); } std::ostringstream stream;