diff --git a/common/formatting/BUILD b/common/formatting/BUILD index 862a32a86..1ab1cbd7e 100644 --- a/common/formatting/BUILD +++ b/common/formatting/BUILD @@ -93,6 +93,7 @@ cc_library( "//common/text:concrete_syntax_leaf", "//common/text:token_info", "//common/util:container_iterator_range", + "//common/util:iterator_adaptors", "//common/util:logging", "//common/util:spacer", "@com_google_absl//absl/base:core_headers", @@ -120,6 +121,7 @@ cc_library( "layout_optimizer.cc", "layout_optimizer_internal.h", ], + hdrs = ["layout_optimizer.h"], deps = [ ":basic_format_style", ":line_wrap_searcher", diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 78dfe381a..725f0f193 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -805,27 +805,6 @@ static std::vector ComputeAlignedRowSpacings( return align_actions; } -// Given a const_iterator and the original mutable container, return -// the corresponding mutable iterator (without resorting to const_cast). -// The 'Container' type is not deducible from function arguments alone. -// TODO(fangism): provide this from common/util/iterator_adaptors. -template -typename Container::iterator ConvertToMutableIterator( - typename Container::const_iterator const_iter, - typename Container::iterator base) { - const typename Container::const_iterator cbase(base); - return base + std::distance(cbase, const_iter); -} - -static MutableFormatTokenRange ConvertToMutableFormatTokenRange( - const FormatTokenRange& const_range, - MutableFormatTokenRange::iterator base) { - using array_type = std::vector; - return MutableFormatTokenRange( - ConvertToMutableIterator(const_range.begin(), base), - ConvertToMutableIterator(const_range.end(), base)); -} - static const AlignmentRow* RightmostSubcolumnWithTokens( const AlignmentRow& node) { if (!node.Value().tokens.empty()) return &node; @@ -861,8 +840,7 @@ static void CommitAlignmentDecisionToRow( } // Tag every subtree as having already been committed to alignment. partition->ApplyPostOrder([](TokenPartitionTree& node) { - node.Value().SetPartitionPolicy( - PartitionPolicyEnum::kSuccessfullyAligned); + node.Value().SetPartitionPolicy(PartitionPolicyEnum::kAlreadyFormatted); }); } } diff --git a/common/formatting/format_token.cc b/common/formatting/format_token.cc index db10c30fe..321dbc066 100644 --- a/common/formatting/format_token.cc +++ b/common/formatting/format_token.cc @@ -23,6 +23,7 @@ #include "common/strings/display_utils.h" #include "common/strings/range.h" #include "common/text/token_info.h" +#include "common/util/iterator_adaptors.h" #include "common/util/logging.h" #include "common/util/spacer.h" @@ -297,4 +298,12 @@ void PreserveSpacesOnDisabledTokenRanges( } } +MutableFormatTokenRange ConvertToMutableFormatTokenRange( + const FormatTokenRange& const_range, + MutableFormatTokenRange::iterator base) { + return MutableFormatTokenRange( + ConvertToMutableIterator(const_range.begin(), base), + ConvertToMutableIterator(const_range.end(), base)); +} + } // namespace verible diff --git a/common/formatting/format_token.h b/common/formatting/format_token.h index a03a59ead..5117b034a 100644 --- a/common/formatting/format_token.h +++ b/common/formatting/format_token.h @@ -175,6 +175,13 @@ using FormatTokenRange = using MutableFormatTokenRange = container_iterator_range::iterator>; +// Given a const_range and a mutable iterator to the original mutable container, +// return the corresponding mutable iterator range (without resorting to +// const_cast). +MutableFormatTokenRange ConvertToMutableFormatTokenRange( + const FormatTokenRange& const_range, + MutableFormatTokenRange::iterator base); + // Enumeration for the final decision about spacing between tokens. // Related enum: SpacingConstraint. // These values are also used during line wrap searching and optimization. diff --git a/common/formatting/layout_optimizer.cc b/common/formatting/layout_optimizer.cc index 0afbcd256..7914c6c41 100644 --- a/common/formatting/layout_optimizer.cc +++ b/common/formatting/layout_optimizer.cc @@ -12,9 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Improve code formatting by optimizing token partitions layout with -// algorithm documented in https://research.google/pubs/pub44667/ -// (similar tool for R language: https://github.com/google/rfmt) +// Implementation of a code layout optimizer described by +// Phillip Yelland in "A New Approach to Optimal Code Formatting" +// (https://research.google/pubs/pub44667/) and originally implemented +// in rfmt (https://github.com/google/rfmt). + +#include "common/formatting/layout_optimizer.h" #include #include @@ -30,6 +33,97 @@ namespace verible { +void OptimizeTokenPartitionTree(const BasicFormatStyle& style, + TokenPartitionTree* node, + std::vector* ftokens) { + CHECK_NOTNULL(node); + + VLOG(4) << __FUNCTION__ << ", before:\n" << *node; + const auto indentation = node->Value().IndentationSpaces(); + + LayoutFunctionFactory factory(style); + + const std::function TraverseTree = + [&TraverseTree, &style, &factory](const TokenPartitionTree& subnode) { + const auto policy = subnode.Value().PartitionPolicy(); + + if (subnode.is_leaf()) { + return factory.Line(subnode.Value()); + } + + switch (policy) { + case PartitionPolicyEnum::kOptimalFunctionCallLayout: { + // Support only function/macro/system calls for now + CHECK_EQ(subnode.Children().size(), 2); + + const auto& function_header = subnode.Children()[0]; + const auto& function_args = subnode.Children()[1]; + + auto header = TraverseTree(function_header); + auto args = TraverseTree(function_args); + + auto stack_layout = factory.Stack({ + header, + factory.Indent(args, style.wrap_spaces), + }); + if (args.MustWrap()) { + return stack_layout; + } + auto juxtaposed_layout = factory.Juxtaposition({ + header, + args, + }); + return factory.Choice({ + std::move(juxtaposed_layout), + std::move(stack_layout), + }); + } + + case PartitionPolicyEnum::kAppendFittingSubPartitions: + case PartitionPolicyEnum::kFitOnLineElseExpand: { + absl::FixedArray layouts(subnode.Children().size()); + std::transform(subnode.Children().begin(), subnode.Children().end(), + layouts.begin(), TraverseTree); + return factory.Wrap(layouts.begin(), layouts.end()); + } + + case PartitionPolicyEnum::kAlwaysExpand: + case PartitionPolicyEnum::kTabularAlignment: { + absl::FixedArray layouts(subnode.Children().size()); + std::transform(subnode.Children().begin(), subnode.Children().end(), + layouts.begin(), TraverseTree); + return factory.Stack(layouts.begin(), layouts.end()); + } + + // TODO(mglb): Think about introducing PartitionPolicies that + // correspond directly to combinators in LayoutFunctionFactory. + // kOptimalFunctionCallLayout strategy could then be implemented + // directly in TreeUnwrapper. It would also allow for proper + // handling of other policies (e.g. kTabularAlignment) in subtrees. + + default: { + LOG(FATAL) << "Unsupported policy: " << policy << "\n" + << "Node:\n" + << subnode; + return LayoutFunction(); + } + } + }; + + const LayoutFunction layout_function = TraverseTree(*node); + CHECK(!layout_function.empty()); + VLOG(4) << __FUNCTION__ << ", layout function:\n" << layout_function; + + auto iter = layout_function.AtOrToTheLeftOf(indentation); + CHECK(iter != layout_function.end()); + VLOG(4) << __FUNCTION__ << ", layout:\n" << iter->layout; + + TreeReconstructor tree_reconstructor(indentation, style); + tree_reconstructor.TraverseTree(iter->layout); + tree_reconstructor.ReplaceTokenPartitionTreeNode(node, ftokens); + VLOG(4) << __FUNCTION__ << ", after:\n" << *node; +} + namespace { // Adopts sublayouts of 'source' into 'destination' if 'source' and @@ -405,4 +499,110 @@ LayoutFunction LayoutFunctionFactory::Choice( return result; } +void TreeReconstructor::TraverseTree(const LayoutTree& layout_tree) { + const auto relative_indentation = layout_tree.Value().IndentationSpaces(); + const ValueSaver indent_saver( + ¤t_indentation_spaces_, + current_indentation_spaces_ + relative_indentation); + // Setting indentation for a line that is going to be appended is invalid and + // probably has been done for some reason that is not going to work as + // intended. + LOG_IF(WARNING, + ((relative_indentation > 0) && (active_unwrapped_line_ != nullptr))) + << "Discarding indentation of a line that's going to be appended."; + + switch (layout_tree.Value().Type()) { + case LayoutType::kLine: { + CHECK(layout_tree.Children().empty()); + if (active_unwrapped_line_ == nullptr) { + auto uwline = layout_tree.Value().ToUnwrappedLine(); + uwline.SetIndentationSpaces(current_indentation_spaces_); + // Prevent SearchLineWraps from processing optimized lines. + uwline.SetPartitionPolicy(PartitionPolicyEnum::kAlreadyFormatted); + active_unwrapped_line_ = &unwrapped_lines_.emplace_back(uwline); + } else { + const auto tokens = layout_tree.Value().ToUnwrappedLine().TokensRange(); + active_unwrapped_line_->SpanUpToToken(tokens.end()); + } + return; + } + + case LayoutType::kJuxtaposition: { + // Append all children + for (const auto& child : layout_tree.Children()) { + TraverseTree(child); + } + return; + } + + case LayoutType::kStack: { + if (layout_tree.Children().empty()) { + return; + } + if (layout_tree.Children().size() == 1) { + TraverseTree(layout_tree.Children().front()); + return; + } + + // Calculate indent for 2nd and further lines. + int indentation = current_indentation_spaces_; + if (active_unwrapped_line_ != nullptr) { + indentation = FitsOnLine(*active_unwrapped_line_, style_).final_column + + layout_tree.Value().SpacesBefore(); + } + + // Append first child + TraverseTree(layout_tree.Children().front()); + + // Put remaining children in their own (indented) lines + const ValueSaver indent_saver(¤t_indentation_spaces_, + indentation); + for (const auto& child : make_range(layout_tree.Children().begin() + 1, + layout_tree.Children().end())) { + active_unwrapped_line_ = nullptr; + TraverseTree(child); + } + return; + } + } +} + +void TreeReconstructor::ReplaceTokenPartitionTreeNode( + TokenPartitionTree* node, std::vector* ftokens) const { + CHECK_NOTNULL(node); + CHECK_NOTNULL(ftokens); + CHECK(!unwrapped_lines_.empty()); + + const auto& first_line = unwrapped_lines_.front(); + const auto& last_line = unwrapped_lines_.back(); + + node->Value() = UnwrappedLine(first_line); + node->Value().SpanUpToToken(last_line.TokensRange().end()); + node->Value().SetIndentationSpaces(current_indentation_spaces_); + node->Value().SetPartitionPolicy( + PartitionPolicyEnum::kOptimalFunctionCallLayout); + + node->Children().clear(); + for (auto& uwline : unwrapped_lines_) { + if (!uwline.IsEmpty()) { + auto line_ftokens = ConvertToMutableFormatTokenRange(uwline.TokensRange(), + ftokens->begin()); + + // Discard first token's original spacing (the partition has already + // proper indentation set). + line_ftokens.front().before.break_decision = SpacingOptions::MustWrap; + line_ftokens.front().before.spaces_required = 0; + line_ftokens.pop_front(); + + for (auto& line_ftoken : line_ftokens) { + SpacingOptions& decision = line_ftoken.before.break_decision; + if (decision == SpacingOptions::Undecided) { + decision = SpacingOptions::MustAppend; + } + } + } + node->AdoptSubtree(uwline); + } +} + } // namespace verible diff --git a/common/formatting/layout_optimizer.h b/common/formatting/layout_optimizer.h new file mode 100644 index 000000000..514de010d --- /dev/null +++ b/common/formatting/layout_optimizer.h @@ -0,0 +1,81 @@ +// Copyright 2017-2021 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Implementation of a code layout optimizer described by +// Phillip Yelland in "A New Approach to Optimal Code Formatting" +// (https://research.google/pubs/pub44667/) and originally implemented +// in rfmt (https://github.com/google/rfmt). + +#ifndef VERIBLE_VERILOG_FORMATTING_LAYOUT_OPTIMIZER_H_ +#define VERIBLE_VERILOG_FORMATTING_LAYOUT_OPTIMIZER_H_ + +#include +#include +#include +#include +#include + +#include "absl/container/fixed_array.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" +#include "common/formatting/basic_format_style.h" +#include "common/formatting/token_partition_tree.h" +#include "common/formatting/unwrapped_line.h" +#include "common/util/vector_tree.h" + +namespace verible { + +// Handles formatting of TokenPartitionTree 'node' that uses +// PartitionPolicyEnum::kOptimalFunctionCallLayout partition policy. +// 'ftokens' is used to get mutable iterators to tokens of formatted partitions. +// The function changes only tokens that are spanned by the passed partitions +// tree. +// +// It is designed to format function calls and requires specific partition +// tree structure: +// +// { +// { ... }, +// { ... } +// } +// +// Nested kOptimalFunctionCallLayout partitions are supported. +// +// EXAMPLE INPUT TREE +// +// Code: `uvm_info(`gfn, $sformatf("%0d %0d\n", cfg.num_pulses, i), UVM_DEBUG) +// +// Partition tree: +// { (>>[...], policy: optimal-function-call-layout) // call: +// { (>>[`uvm_info (]) } // - header +// { (>>>>>>[...]) // - arguments: +// { (>>>>>>[`gfn ,]) } // - (arg) +// { (>>>>>>[...], policy: optimal-function-call-layout) // nested call: +// { (>>>>>>[$sformatf (]) } // - header +// { (>>>>>>>>>>[...]) // - arguments +// { (>>>>>>>>>>["%0d %0d\n" ,]) } // - (arg) +// { (>>>>>>>>>>[cfg . num_pulses ,]) } // - (arg) +// { (>>>>>>>>>>[i ) ,]) } // - (arg) +// } +// } +// { (>>>>>>[UVM_DEBUG )]) } // - (arg) +// } +// } +void OptimizeTokenPartitionTree(const BasicFormatStyle& style, + TokenPartitionTree* node, + std::vector* ftokens); + +} // namespace verible + +#endif // VERIBLE_VERILOG_FORMATTING_LAYOUT_OPTIMIZER_H_ diff --git a/common/formatting/layout_optimizer_internal.h b/common/formatting/layout_optimizer_internal.h index f419f53c5..08c93dd30 100644 --- a/common/formatting/layout_optimizer_internal.h +++ b/common/formatting/layout_optimizer_internal.h @@ -70,6 +70,8 @@ class LayoutItem { must_wrap_(!tokens_.empty() && tokens_.front().before.break_decision == SpacingOptions::MustWrap) {} + // Multiple LayoutFunctionSegments can store copies of the same layout. + // The objects are copied mostly in LayoutFunctionFactory::* functions. LayoutItem(const LayoutItem&) = default; LayoutItem& operator=(const LayoutItem&) = default; @@ -78,15 +80,17 @@ class LayoutItem { LayoutType Type() const { return type_; } - // Returns "hard" indent, which is never removed when the layout is joined - // with another layout. + // Indentation used for a layout when it is placed at the beginning of a line. + // Effective indentation in this case is a sum of the item's and its + // ancestors' indetation int IndentationSpaces() const { return indentation_; } - // Sets "hard" indent, which is never removed when the layout is joined - // with another layout. + // Sets indentation used for a layout when it is placed at the beginning of + // a line. void SetIndentationSpaces(int indent) { indentation_ = indent; } - // Returns amount of spaces before first token. + // Returns number of spaces required before the first token. The spaces are + // used when the layout is appended to a non-empty line. int SpacesBefore() const { return spaces_before_; } // Returns whether to force line break just before this layout. @@ -123,7 +127,6 @@ class LayoutItem { UnwrappedLine uwline(0, tokens_.begin()); uwline.SpanUpToToken(tokens_.end()); - uwline.SetPartitionPolicy(PartitionPolicyEnum::kAlwaysExpand); return uwline; } @@ -618,6 +621,32 @@ class LayoutFunctionFactory { const BasicFormatStyle& style_; }; +class TreeReconstructor { + public: + TreeReconstructor(int indentation_spaces, const BasicFormatStyle& style) + : current_indentation_spaces_(indentation_spaces), style_(style) {} + ~TreeReconstructor() = default; + + TreeReconstructor(const TreeReconstructor&) = delete; + TreeReconstructor(TreeReconstructor&&) = delete; + TreeReconstructor& operator=(const TreeReconstructor&) = delete; + TreeReconstructor& operator=(TreeReconstructor&&) = delete; + + void TraverseTree(const LayoutTree& layout_tree); + + void ReplaceTokenPartitionTreeNode( + TokenPartitionTree* node, std::vector* ftokens) const; + + private: + std::vector unwrapped_lines_; + + UnwrappedLine* active_unwrapped_line_ = nullptr; + + int current_indentation_spaces_; + + const BasicFormatStyle& style_; +}; + } // namespace verible #endif // VERIBLE_VERILOG_FORMATTING_LAYOUT_OPTIMIZER_INTERNAL_H_ diff --git a/common/formatting/layout_optimizer_test.cc b/common/formatting/layout_optimizer_test.cc index a35847fcd..2346d3bca 100644 --- a/common/formatting/layout_optimizer_test.cc +++ b/common/formatting/layout_optimizer_test.cc @@ -11,6 +11,8 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. +// +#include "common/formatting/layout_optimizer.h" #include #include @@ -47,6 +49,10 @@ TEST(LayoutTypeTest, ToString) { EXPECT_EQ(ToString(static_cast(-1)), "???"); } +bool TokenRangeEqual(const UnwrappedLine& left, const UnwrappedLine& right) { + return left.TokensRange() == right.TokensRange(); +} + class LayoutTest : public ::testing::Test, public UnwrappedLineMemoryHandler { public: LayoutTest() @@ -153,8 +159,6 @@ TEST_F(LayoutTest, AsUnwrappedLine) { const auto uwline = layout_short.ToUnwrappedLine(); EXPECT_EQ(uwline.IndentationSpaces(), 0); - EXPECT_EQ(uwline.PartitionPolicy(), PartitionPolicyEnum::kAlwaysExpand); - EXPECT_EQ(uwline.TokensRange().begin(), short_line.TokensRange().begin()); EXPECT_EQ(uwline.TokensRange().end(), short_line.TokensRange().end()); } @@ -1814,5 +1818,472 @@ TEST_F(LayoutFunctionFactoryTest, IndentWithOtherCombinators) { } } +class TreeReconstructorTest : public ::testing::Test, + public UnwrappedLineMemoryHandler { + public: + TreeReconstructorTest() + : sample_("first_line second_line third_line fourth_line"), + tokens_(absl::StrSplit(sample_, ' ')) { + for (const auto token : tokens_) { + ftokens_.emplace_back(TokenInfo{1, token}); + } + CreateTokenInfos(ftokens_); + } + + protected: + const std::string sample_; + const std::vector tokens_; + std::vector ftokens_; +}; + +TEST_F(TreeReconstructorTest, SingleLine) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine single_line(0, begin); + single_line.SpanUpToToken(begin + 1); + + const auto layout_tree = LayoutTree(LayoutItem(single_line)); + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree(UnwrappedLine(0, begin)); + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{single_line, // + Tree{single_line}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, HorizontalLayoutWithOneLine) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine uwline(0, begin); + uwline.SpanUpToToken(begin + 1); + + const auto layout_tree = + LayoutTree(LayoutItem(LayoutType::kJuxtaposition, 0, false), + LayoutTree(LayoutItem(uwline))); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{uwline, // + Tree{uwline}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, HorizontalLayoutSingleLines) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine left_line(0, begin); + left_line.SpanUpToToken(begin + 1); + UnwrappedLine right_line(0, begin + 1); + right_line.SpanUpToToken(begin + 2); + UnwrappedLine all(0, left_line.TokensRange().begin()); + all.SpanUpToToken(right_line.TokensRange().end()); + + const auto layout_tree = LayoutTree( + LayoutItem(LayoutType::kJuxtaposition, 0, + left_line.TokensRange().front().before.break_decision == + SpacingOptions::MustWrap), + LayoutTree(LayoutItem(left_line)), LayoutTree(LayoutItem(right_line))); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree(UnwrappedLine(0, begin)); + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected(all, // + Tree(all)); + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, EmptyHorizontalLayout) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine upper_line(0, begin); + upper_line.SpanUpToToken(begin + 1); + UnwrappedLine lower_line(0, begin + 1); + lower_line.SpanUpToToken(begin + 2); + UnwrappedLine all(0, upper_line.TokensRange().begin()); + all.SpanUpToToken(lower_line.TokensRange().end()); + + const auto layout_tree = + LayoutTree(LayoutItem(LayoutType::kJuxtaposition, 0, false), + LayoutTree(LayoutItem(upper_line)), + LayoutTree(LayoutItem(LayoutType::kJuxtaposition, 0, false)), + LayoutTree(LayoutItem(lower_line))); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{all, // + Tree{all}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, VerticalLayoutWithOneLine) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine uwline(0, begin); + uwline.SpanUpToToken(begin + 1); + + const auto layout_tree = LayoutTree(LayoutItem(LayoutType::kStack, 0, false), + LayoutTree(LayoutItem(uwline))); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{uwline, // + Tree{uwline}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, VerticalLayoutSingleLines) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine upper_line(0, begin); + upper_line.SpanUpToToken(begin + 1); + UnwrappedLine lower_line(0, begin + 1); + lower_line.SpanUpToToken(begin + 2); + UnwrappedLine all(0, upper_line.TokensRange().begin()); + all.SpanUpToToken(lower_line.TokensRange().end()); + + const auto layout_tree = LayoutTree( + LayoutItem(LayoutType::kStack, 0, + upper_line.TokensRange().front().before.break_decision == + SpacingOptions::MustWrap), + LayoutTree(LayoutItem(upper_line)), LayoutTree(LayoutItem(lower_line))); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{all, // + Tree{upper_line}, // + Tree{lower_line}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, EmptyVerticalLayout) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine upper_line(0, begin); + upper_line.SpanUpToToken(begin + 1); + UnwrappedLine lower_line(0, begin + 1); + lower_line.SpanUpToToken(begin + 2); + UnwrappedLine all(0, upper_line.TokensRange().begin()); + all.SpanUpToToken(lower_line.TokensRange().end()); + + const auto layout_tree = + LayoutTree(LayoutItem(LayoutType::kStack, 0, false), + LayoutTree(LayoutItem(upper_line)), + LayoutTree(LayoutItem(LayoutType::kStack, 0, false)), + LayoutTree(LayoutItem(lower_line))); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{all, // + Tree{upper_line}, // + Tree{lower_line}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, VerticallyJoinHorizontalLayouts) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine first_line(0, begin); + first_line.SpanUpToToken(begin + 1); + UnwrappedLine second_line(0, begin + 1); + second_line.SpanUpToToken(begin + 2); + UnwrappedLine third_line(0, begin + 2); + third_line.SpanUpToToken(begin + 3); + UnwrappedLine fourth_line(0, begin + 3); + fourth_line.SpanUpToToken(begin + 4); + + UnwrappedLine upper_line(0, first_line.TokensRange().begin()); + upper_line.SpanUpToToken(second_line.TokensRange().end()); + UnwrappedLine lower_line(0, third_line.TokensRange().begin()); + lower_line.SpanUpToToken(fourth_line.TokensRange().end()); + + UnwrappedLine all(0, upper_line.TokensRange().begin()); + all.SpanUpToToken(lower_line.TokensRange().end()); + const LayoutTree layout_tree{ + LayoutItem(LayoutType::kStack, 0, false), + LayoutTree(LayoutItem(LayoutType::kJuxtaposition, 0, false), + LayoutTree(LayoutItem(first_line)), + LayoutTree(LayoutItem(second_line))), + LayoutTree(LayoutItem(LayoutType::kJuxtaposition, 0, false), + LayoutTree(LayoutItem(third_line)), + LayoutTree(LayoutItem(fourth_line)))}; + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{all, // + Tree{upper_line}, // + Tree{lower_line}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, HorizontallyJoinVerticalLayouts) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine first_line(0, begin); + first_line.SpanUpToToken(begin + 1); + UnwrappedLine second_line(0, begin + 1); + second_line.SpanUpToToken(begin + 2); + UnwrappedLine third_line(0, begin + 2); + third_line.SpanUpToToken(begin + 3); + UnwrappedLine fourth_line(0, begin + 3); + fourth_line.SpanUpToToken(begin + 4); + + UnwrappedLine upper_line(0, first_line.TokensRange().begin()); + upper_line.SpanUpToToken(first_line.TokensRange().end()); + UnwrappedLine middle_line(0, second_line.TokensRange().begin()); + middle_line.SpanUpToToken(third_line.TokensRange().end()); + UnwrappedLine bottom_line(0, fourth_line.TokensRange().begin()); + bottom_line.SpanUpToToken(fourth_line.TokensRange().end()); + + UnwrappedLine all(0, upper_line.TokensRange().begin()); + all.SpanUpToToken(bottom_line.TokensRange().end()); + + const LayoutTree layout_tree{ + LayoutItem(LayoutType::kJuxtaposition, 0, false), + LayoutTree(LayoutItem(LayoutType::kStack, 0, false), + LayoutTree(LayoutItem(first_line)), + LayoutTree(LayoutItem(second_line))), + LayoutTree(LayoutItem(LayoutType::kStack, 0, false), + LayoutTree(LayoutItem(third_line)), + LayoutTree(LayoutItem(fourth_line)))}; + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{all, // + Tree{upper_line}, // + Tree{middle_line}, // + Tree{bottom_line}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; +} + +TEST_F(TreeReconstructorTest, IndentSingleLine) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + BasicFormatStyle style; + + UnwrappedLine single_line(0, begin); + single_line.SpanUpToToken(begin + 1); + + const auto indent = 7; + LayoutTree layout_tree{LayoutItem(single_line)}; + layout_tree.Value().SetIndentationSpaces(indent); + + TreeReconstructor tree_reconstructor(0, style); + tree_reconstructor.TraverseTree(layout_tree); + + auto optimized_tree = TokenPartitionTree{UnwrappedLine(0, begin)}; + tree_reconstructor.ReplaceTokenPartitionTreeNode(&optimized_tree, + &pre_format_tokens_); + + using Tree = TokenPartitionTree; + const Tree tree_expected{single_line, // + Tree{single_line}}; + const auto diff = DeepEqual(optimized_tree, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << optimized_tree << "\n"; + + EXPECT_EQ(optimized_tree.Children()[0].Value().IndentationSpaces(), indent); +} + +class OptimizeTokenPartitionTreeTest : public ::testing::Test, + public UnwrappedLineMemoryHandler { + public: + OptimizeTokenPartitionTreeTest() + : sample_( + "function_fffffffffff( type_a_aaaa, " + "type_b_bbbbb, type_c_cccccc, " + "type_d_dddddddd, type_e_eeeeeeee, type_f_ffff);"), + tokens_(absl::StrSplit(sample_, ' ')) { + for (const auto token : tokens_) { + ftokens_.emplace_back(TokenInfo{1, token}); + } + CreateTokenInfos(ftokens_); + } + + protected: + const std::string sample_; + const std::vector tokens_; + std::vector ftokens_; +}; + +TEST_F(OptimizeTokenPartitionTreeTest, OneLevelFunctionCall) { + const auto& preformat_tokens = pre_format_tokens_; + const auto begin = preformat_tokens.begin(); + + UnwrappedLine function_name(0, begin); + function_name.SpanUpToToken(begin + 1); + UnwrappedLine arg_a(0, begin + 1); + arg_a.SpanUpToToken(begin + 2); + UnwrappedLine arg_b(0, begin + 2); + arg_b.SpanUpToToken(begin + 3); + UnwrappedLine arg_c(0, begin + 3); + arg_c.SpanUpToToken(begin + 4); + UnwrappedLine arg_d(0, begin + 4); + arg_d.SpanUpToToken(begin + 5); + UnwrappedLine arg_e(0, begin + 5); + arg_e.SpanUpToToken(begin + 6); + UnwrappedLine arg_f(0, begin + 6); + arg_f.SpanUpToToken(begin + 7); + + function_name.SetPartitionPolicy(PartitionPolicyEnum::kAlwaysExpand); + arg_a.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + arg_b.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + arg_c.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + arg_d.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + arg_e.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + arg_f.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + + UnwrappedLine header(0, function_name.TokensRange().begin()); + header.SpanUpToToken(function_name.TokensRange().end()); + UnwrappedLine args(0, arg_a.TokensRange().begin()); + args.SpanUpToToken(arg_f.TokensRange().end()); + + header.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + args.SetPartitionPolicy(PartitionPolicyEnum::kFitOnLineElseExpand); + + UnwrappedLine all(0, header.TokensRange().begin()); + all.SpanUpToToken(args.TokensRange().end()); + all.SetPartitionPolicy(PartitionPolicyEnum::kOptimalFunctionCallLayout); + + using Tree = TokenPartitionTree; + Tree tree_under_test{all, // + Tree{header}, // + Tree{args, // + Tree{arg_a}, // + Tree{arg_b}, // + Tree{arg_c}, // + Tree{arg_d}, // + Tree{arg_e}, // + Tree{arg_f}}}; + + BasicFormatStyle style; + style.column_limit = 40; + OptimizeTokenPartitionTree(style, &tree_under_test, &pre_format_tokens_); + + UnwrappedLine args_top_line(0, arg_a.TokensRange().begin()); + args_top_line.SpanUpToToken(arg_b.TokensRange().end()); + UnwrappedLine args_middle_line(0, arg_c.TokensRange().begin()); + args_middle_line.SpanUpToToken(arg_d.TokensRange().end()); + UnwrappedLine args_bottom_line(0, arg_e.TokensRange().begin()); + args_bottom_line.SpanUpToToken(arg_f.TokensRange().end()); + + const Tree tree_expected{all, // + Tree{header}, // + Tree{args_top_line}, // + Tree{args_middle_line}, // + Tree{args_bottom_line}}; + + const auto diff = DeepEqual(tree_under_test, tree_expected, TokenRangeEqual); + EXPECT_EQ(diff.left, nullptr) << "Expected:\n" + << tree_expected << "\nGot:\n" + << tree_under_test << "\n"; + + // header + EXPECT_EQ(tree_under_test.Children()[0].Value().IndentationSpaces(), 0); + // args_top_line (wrapped) + EXPECT_EQ(tree_under_test.Children()[1].Value().IndentationSpaces(), 4); + // args_middle_line (wrapped) + EXPECT_EQ(tree_under_test.Children()[2].Value().IndentationSpaces(), 4); + // args_bottom_line (wrapped) + EXPECT_EQ(tree_under_test.Children()[3].Value().IndentationSpaces(), 4); +} + } // namespace } // namespace verible diff --git a/common/formatting/unwrapped_line.cc b/common/formatting/unwrapped_line.cc index 9cda5cca9..ea67e6082 100644 --- a/common/formatting/unwrapped_line.cc +++ b/common/formatting/unwrapped_line.cc @@ -42,10 +42,12 @@ std::ostream& operator<<(std::ostream& stream, PartitionPolicyEnum p) { return stream << "fit-else-expand"; case PartitionPolicyEnum::kTabularAlignment: return stream << "tabular-alignment"; - case PartitionPolicyEnum::kSuccessfullyAligned: - return stream << "aligned-success"; + case PartitionPolicyEnum::kAlreadyFormatted: + return stream << "already-formatted"; case PartitionPolicyEnum::kAppendFittingSubPartitions: return stream << "append-fitting-sub-partitions"; + case PartitionPolicyEnum::kOptimalFunctionCallLayout: + return stream << "optimal-function-call-layout"; } LOG(FATAL) << "Unknown partition policy " << int(p); } diff --git a/common/formatting/unwrapped_line.h b/common/formatting/unwrapped_line.h index 3f39513e2..6b7ac5550 100644 --- a/common/formatting/unwrapped_line.h +++ b/common/formatting/unwrapped_line.h @@ -51,18 +51,22 @@ enum class PartitionPolicyEnum { // columns that use space-padding to achieve vertical alignment. kTabularAlignment, - // Signal that this unwrapped line (a direct child of a partition marked with - // kTabularAlignment) has been successfully aligned with spacing padded. - // In this case, do NOT bother to call SearchLineWraps or perform any other - // spacing/wrapping optimization. - // Reserved for setting only from align.cc. - kSuccessfullyAligned, + // Signal that this unwrapped line has been successfully formatted with + // spacing added. In this case, do NOT bother to call SearchLineWraps or + // perform any other spacing/wrapping optimization. + // Reserved for setting only from policy-specific formatters, like Layout + // Optimizer and Tabular Aligner. + kAlreadyFormatted, // Treats subpartitions as units, and appends them to the same line as // long as they fit, else wrap them aligned to the position of the first // element It uses first subpartition length to compute indentation spaces or // FormatStyle.wrap_spaces when wrapping. kAppendFittingSubPartitions, + + // Does optimizations on partition tree and its subpartitions. + // Currently it used for reshaping function calls partitions. + kOptimalFunctionCallLayout, }; std::ostream& operator<<(std::ostream&, PartitionPolicyEnum); diff --git a/common/util/iterator_adaptors.h b/common/util/iterator_adaptors.h index 1e314c348..2915854d6 100644 --- a/common/util/iterator_adaptors.h +++ b/common/util/iterator_adaptors.h @@ -50,6 +50,15 @@ reversed_view(T& t) { // in std:: when compiling with C++14 or newer. } +// Given a const_iterator and a mutable iterator to the original mutable +// container, return the corresponding mutable iterator (without resorting to +// const_cast). +template +Iter ConvertToMutableIterator(ConstIter const_iter, Iter base) { + auto cbase = ConstIter(base); + return base + std::distance(cbase, const_iter); +} + } // namespace verible #endif // VERIBLE_COMMON_UTIL_ITERATOR_ADAPTORS_H_ diff --git a/common/util/iterator_adaptors_test.cc b/common/util/iterator_adaptors_test.cc index cac832c4b..cc0b63815 100644 --- a/common/util/iterator_adaptors_test.cc +++ b/common/util/iterator_adaptors_test.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include "gmock/gmock.h" @@ -85,5 +86,16 @@ TEST(ReversedViewTest, NonEmptySet) { EXPECT_THAT(reversed_view(v), ElementsAre(7, 6, 3)); } +TEST(ConvertToMutableIteratorTest, Convert) { + std::vector v{3, 6, 7}; + const std::vector::const_iterator const_it = v.cbegin() + 1; + auto it = ConvertToMutableIterator(const_it, v.begin()); + static_assert(std::is_same_v::iterator>); + EXPECT_EQ(*it, 6); + *it = 42; + EXPECT_EQ(*it, 42); + EXPECT_EQ(v[1], 42); +} + } // namespace } // namespace verible diff --git a/verilog/formatting/BUILD b/verilog/formatting/BUILD index 8f0262a5e..b3378948f 100644 --- a/verilog/formatting/BUILD +++ b/verilog/formatting/BUILD @@ -137,6 +137,7 @@ cc_library( ":tree_unwrapper", "//common/formatting:format_token", "//common/formatting:line_wrap_searcher", + "//common/formatting:layout_optimizer", "//common/formatting:token_partition_tree", "//common/formatting:unwrapped_line", "//common/formatting:verification", diff --git a/verilog/formatting/formatter.cc b/verilog/formatting/formatter.cc index babc2babc..2319f3b35 100644 --- a/verilog/formatting/formatter.cc +++ b/verilog/formatting/formatter.cc @@ -22,6 +22,7 @@ #include "absl/status/status.h" #include "common/formatting/format_token.h" +#include "common/formatting/layout_optimizer.h" #include "common/formatting/line_wrap_searcher.h" #include "common/formatting/token_partition_tree.h" #include "common/formatting/unwrapped_line.h" @@ -355,6 +356,7 @@ static void DeterminePartitionExpansion( LOG(FATAL) << "Got an uninitialized partition policy at: " << uwline; break; } + case PartitionPolicyEnum::kOptimalFunctionCallLayout: case PartitionPolicyEnum::kAlwaysExpand: { if (children.size() > 1) { node_view.Expand(); @@ -381,7 +383,7 @@ static void DeterminePartitionExpansion( break; } - case PartitionPolicyEnum::kSuccessfullyAligned: + case PartitionPolicyEnum::kAlreadyFormatted: VLOG(3) << "Aligned fits, un-expanding."; node_view.Unexpand(); break; @@ -746,6 +748,10 @@ Status Formatter::Format(const ExecutionControl& control) { // Reshape partition tree with kAppendFittingSubPartitions policy verible::ReshapeFittingSubpartitions(&node, style_); break; + case PartitionPolicyEnum::kOptimalFunctionCallLayout: + verible::OptimizeTokenPartitionTree( + style_, &node, &unwrapper_data.preformatted_tokens); + break; case PartitionPolicyEnum::kTabularAlignment: // TODO(b/145170750): Adjust inter-token spacing to achieve alignment, // but leave partitioning intact. @@ -777,7 +783,7 @@ Status Formatter::Format(const ExecutionControl& control) { // uwline.PartitionPolicy(). if (continuation_comment_aligner.HandleLine(uwline, &formatted_lines_)) { } else if (uwline.PartitionPolicy() == - PartitionPolicyEnum::kSuccessfullyAligned) { + PartitionPolicyEnum::kAlreadyFormatted) { // For partitions that were successfully aligned, do not search // line-wrapping, but instead accept the adjusted padded spacing. formatted_lines_.emplace_back(uwline); diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index 59cff5917..9ae81a0f8 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -6683,7 +6683,8 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { " `ppgJH3JoxhwyTmZ2dgPiuMQzpRAWiSs(\n" " {xYtxuh6.FIMcVPEWfhtoI2FSe, xYtxuh6.ZVL5XASVGLYz32} == " "SqRgavM[15:2];\n" - "JgQLBG == 4'h0;, \"foo\")\n" + "JgQLBG == 4'h0;,\n" + " \"foo\")\n" "endtask\n", }, @@ -11750,6 +11751,283 @@ TEST(FormatterEndToEndTest, PortDeclarationDimensionsAlignmentTest) { // TODO(fangism): directed tests using style variations +static constexpr FormatterTestCase kNestedFunctionsTestCases40ColumnsLimit[] = { + {"module foo;" + "`uvm_info(`gfn, $sformatf(\n" + "\"\\n base_vseq: generate %0d pulse in channel %0d\", cfg.num_pulses, " + "i), UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(\n" + " `gfn,\n" + " $sformatf(\n" + " \"\\n base_vseq: generate %0d pulse in channel %0d\",\n" + " cfg.num_pulses, i), UVM_DEBUG)\n" + "endmodule\n"}, + {"module foo;`uvm_info(`gfn, $sformatf(" + "\"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE |----\\n %s\"," + "cfg.convert2string()), UVM_LOW)\n" + "endmodule", + "module foo;\n" + " `uvm_info(\n" + " `gfn,\n" + " $sformatf(\n" + " \"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE |----\\n %s\",\n" + " cfg.convert2string()),\n" + " UVM_LOW)\n" // FIXME: Wrapped by SearchLineWraps + "endmodule\n"}, +}; + +TEST(FormatterEndToEndTest, FormatNestedFunctionsTestCases40ColumnsLimit) { + // Use a fixed style. + FormatStyle style; + style.column_limit = 40; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + + for (const auto& test_case : kNestedFunctionsTestCases40ColumnsLimit) { + 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; + } +} + +static constexpr FormatterTestCase kNestedFunctionsTestCases60ColumnsLimit[] = { + {"module foo;" + "`uvm_info(`gfn, $sformatf(\n" + "\"\\n base_vseq: generate %0d pulse in channel %0d\", cfg.num_pulses, " + "i), UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(\n" + " `gfn,\n" + " $sformatf(\n" + " \"\\n base_vseq: generate %0d pulse in channel %0d\",\n" + " cfg.num_pulses, i), UVM_DEBUG)\n" + "endmodule\n"}, + {"module foo;`uvm_info(`gfn, $sformatf(" + "\"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE |----\\n %s\"," + "cfg.convert2string()), UVM_LOW)\n" + "endmodule", + "module foo;\n" + " `uvm_info(\n" + " `gfn,\n" + " $sformatf(\n" + " \"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE |----\\n %s\",\n" + " cfg.convert2string()), UVM_LOW)\n" + "endmodule\n"}, +}; + +TEST(FormatterEndToEndTest, FormatNestedFunctionsTestCases60ColumnsLimit) { + // Use a fixed style. + FormatStyle style; + style.column_limit = 60; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + + for (const auto& test_case : kNestedFunctionsTestCases60ColumnsLimit) { + 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; + } +} + +static constexpr FormatterTestCase kNestedFunctionsTestCases80ColumnsLimit[] = { + {"module foo;" + "`uvm_info(`gfn, $sformatf(\n" + "\"\\n base_vseq: generate %0d pulse in channel %0d\", cfg.num_pulses, " + "i), UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(`gfn, $sformatf(\"\\n base_vseq: generate %0d pulse in " + "channel %0d\",\n" + " cfg.num_pulses, i), UVM_DEBUG)\n" + "endmodule\n"}, + {"module foo;`uvm_info(`gfn, $sformatf(" + "\"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE |----\\n %s\"," + "cfg.convert2string()), UVM_LOW)\n" + "endmodule", + "module foo;\n" + " `uvm_info(\n" + " `gfn, $sformatf(\"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE " + "|----\\n %s\",\n" + " cfg.convert2string()), UVM_LOW)\n" + "endmodule\n"}, + {"module x;" + "`uvm_fatal(`gfn, $sformatf(" + "\"The data 0x%0h written to the signature address is formatted " + "incorrectly.\"," + "signature_data))\n" + "endmodule", + "module x;\n" + " `uvm_fatal(\n" + " `gfn,\n" + " $sformatf(\n" + " \"The data 0x%0h written to the signature address is formatted " + "incorrectly.\",\n" + " signature_data))\n" + "endmodule\n"}, +}; + +TEST(FormatterEndToEndTest, FormatNestedFunctionsTestCases80ColumnsLimit) { + // Use a fixed style. + FormatStyle style; + style.column_limit = 80; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + + for (const auto& test_case : kNestedFunctionsTestCases80ColumnsLimit) { + 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; + } +} + +static constexpr FormatterTestCase kNestedFunctionsTestCases100ColumnsLimit[] = + { + {"module foo;" + "`uvm_info(`gfn, $sformatf(\n" + "\"\\n base_vseq: generate %0d pulse in channel %0d\", " + "cfg.num_pulses, i), UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(`gfn, $sformatf(\"\\n base_vseq: generate %0d pulse in " + "channel %0d\", cfg.num_pulses, i),\n" + " UVM_DEBUG)\n" + "endmodule\n"}, + {"module foo;`uvm_info(`gfn, $sformatf(" + "\"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE |----\\n %s\"," + "cfg.convert2string()), UVM_LOW)\n" + "endmodule", + "module foo;\n" + " `uvm_info(\n" + " `gfn, $sformatf(\"\\n\\n\\t ----| STARTING AES MAIN SEQUENCE " + "|----\\n %s\", cfg.convert2string()),\n" + " UVM_LOW)\n" + "endmodule\n"}, + {"module x;" + "`uvm_fatal(`gfn, $sformatf(" + "\"The data 0x%0h written to the signature address is formatted " + "incorrectly.\"," + "signature_data))\n" + "endmodule", + "module x;\n" + " `uvm_fatal(\n" + " `gfn, $sformatf(\"The data 0x%0h written to the signature " + "address is formatted incorrectly.\",\n" + " signature_data))\n" + "endmodule\n"}, + {// nested modules, three-levels + "module x; module y; module z;" + "`uvm_fatal(`gfn, $sformatf(" + "\"The data 0x%0h written to the signature address is formatted " + "incorrectly.\"," + "signature_data))\n" + "endmodule endmodule endmodule", + "module x;\n" + " module y;\n" + " module z;\n" + " `uvm_fatal(\n" + " `gfn,\n" + " $sformatf(\"The data 0x%0h written to the signature " + "address is formatted incorrectly.\",\n" + " signature_data))\n" + " endmodule\n" + " endmodule\n" + "endmodule\n"}, +}; + +TEST(FormatterEndToEndTest, FormatNestedFunctionsTestCases100ColumnsLimit) { + // Use a fixed style. + FormatStyle style; + style.column_limit = 100; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + + for (const auto& test_case : kNestedFunctionsTestCases100ColumnsLimit) { + 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; + } +} + +static constexpr FormatterTestCase kFunctionCallsWithComments[] = { + {// no comments + "module foo;\n" + "`uvm_info(`gfn, \"xx\",\n" + "cfg.num_pulses, i, UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(`gfn, \"xx\", cfg.num_pulses, i, UVM_DEBUG)\n" + "endmodule\n"}, + {// one comment + "module foo;\n" + "`uvm_info(`gfn, \"xx\", // yy\n" + "cfg.num_pulses, i, UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(`gfn, \"xx\", // yy\n" + " cfg.num_pulses, i, UVM_DEBUG)\n" + "endmodule\n"}, + {// two comments + "module foo;\n" + "`uvm_info(`gfn, \"xx\", " + " cfg.num_pulses, // xx\n" + " i, UVM_DEBUG)// uuu\n" + "endmodule", + "module foo;\n" + " `uvm_info(`gfn, \"xx\", cfg.num_pulses, // xx\n" + " i, UVM_DEBUG) // uuu\n" + "endmodule\n"}, + + {// nested function calls with comments + "module foo;" + "`uvm_info(`gfn, $sformatf(\"\\n base_vseq: generate %0d" + " pulse in channel %0d\", cfg.num_pulses, // comment\n" + " i), UVM_DEBUG)\n" + "endmodule", + "module foo;\n" + " `uvm_info(`gfn, $sformatf(\"\\n base_vseq: generate %0d pulse in " + "channel %0d\",\n" + " cfg.num_pulses, // comment\n" + " i), UVM_DEBUG)\n" + "endmodule\n"}, +}; + +TEST(FormatterEndToEndTest, FunctionCallsWithComments) { + // Use a fixed style. + FormatStyle style; + style.column_limit = 100; + style.indentation_spaces = 2; + style.wrap_spaces = 4; + + for (const auto& test_case : kFunctionCallsWithComments) { + 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; + } +} + } // namespace } // namespace formatter } // namespace verilog diff --git a/verilog/formatting/tree_unwrapper.cc b/verilog/formatting/tree_unwrapper.cc index a6c7b3578..d97b5e1a3 100644 --- a/verilog/formatting/tree_unwrapper.cc +++ b/verilog/formatting/tree_unwrapper.cc @@ -930,8 +930,29 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions( } case NodeEnum::kSystemTFCall: { - // TODO(fangism): Create own section only for standalone calls - if (Context().DirectParentIs(NodeEnum::kStatement)) { + // TODO(ldk): Targeting into one-level nested call, + // need to expand it into more general solution + if (Context().DirectParentsAre( + {NodeEnum::kExpression, NodeEnum::kMacroArgList, + NodeEnum::kParenGroup, NodeEnum::kMacroCall, + NodeEnum::kModuleItemList})) { + VisitIndentedSection(node, 0, + PartitionPolicyEnum::kOptimalFunctionCallLayout); + } else if (Context().DirectParentsAre( + {NodeEnum::kExpression, NodeEnum::kMacroArgList, + NodeEnum::kParenGroup, NodeEnum::kMacroCall, + NodeEnum::kStatementList, NodeEnum::kTaskDeclaration})) { + VisitIndentedSection(node, 0, + PartitionPolicyEnum::kOptimalFunctionCallLayout); + } else if (Context().DirectParentsAre( + {NodeEnum::kExpression, NodeEnum::kMacroArgList, + NodeEnum::kParenGroup, NodeEnum::kMacroCall, + NodeEnum::kBlockItemStatementList, NodeEnum::kSeqBlock, + NodeEnum::kIfBody, NodeEnum::kIfClause})) { + VisitIndentedSection(node, 0, + PartitionPolicyEnum::kOptimalFunctionCallLayout); + // TODO(fangism): Create own section only for standalone calls + } else if (Context().DirectParentIs(NodeEnum::kStatement)) { VisitIndentedSection(node, 0, PartitionPolicyEnum::kAppendFittingSubPartitions); } else { @@ -946,8 +967,17 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions( const int indent = ShouldIndentRelativeToDirectParent(Context()) ? style_.indentation_spaces : 0; - VisitIndentedSection(node, indent, - PartitionPolicyEnum::kAppendFittingSubPartitions); + // Apply optimal formatting just for standalone macro calls (for now) + const auto policy = + Context().DirectParentIs(NodeEnum::kModuleItemList) || + Context().DirectParentsAre( + {NodeEnum::kStatementList, NodeEnum::kTaskDeclaration}) || + Context().DirectParentsAre( + {NodeEnum::kBlockItemStatementList, NodeEnum::kSeqBlock, + NodeEnum::kIfBody, NodeEnum::kIfClause}) + ? PartitionPolicyEnum::kOptimalFunctionCallLayout + : PartitionPolicyEnum::kAppendFittingSubPartitions; + VisitIndentedSection(node, indent, policy); break; } @@ -1913,8 +1943,13 @@ void TreeUnwrapper::ReshapeTokenPartitions( case NodeEnum::kRandomizeFunctionCall: case NodeEnum::kSystemTFCall: { - if (partition.Value().PartitionPolicy() == - PartitionPolicyEnum::kAppendFittingSubPartitions) { + // Function/system calls don't always have it's own section. + // So before we do any adjuments we check that. + // We're checking for partition policy because those two policies + // force own partition section. + const auto policy = partition.Value().PartitionPolicy(); + if (policy == PartitionPolicyEnum::kAppendFittingSubPartitions || + policy == PartitionPolicyEnum::kOptimalFunctionCallLayout) { auto& last = *ABSL_DIE_IF_NULL(partition.RightmostDescendant()); if (PartitionStartsWithCloseParen(last) || PartitionStartsWithSemicolon(last)) {