From 25bfc8ca5703089d4f8342d12e9452ea63117548 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Sat, 10 Jul 2021 14:27:07 +0200 Subject: [PATCH 01/23] Add VectorTree iterators Adds code that allows to traverse VectorTree in specific order using range for loop. Supported traversals: * Leaves * Pre order * Post order --- common/util/BUILD | 21 ++ common/util/vector_tree_iterators.h | 178 +++++++++++ common/util/vector_tree_iterators_test.cc | 370 ++++++++++++++++++++++ 3 files changed, 569 insertions(+) create mode 100644 common/util/vector_tree_iterators.h create mode 100644 common/util/vector_tree_iterators_test.cc diff --git a/common/util/BUILD b/common/util/BUILD index b0766c93f..51c47a16e 100644 --- a/common/util/BUILD +++ b/common/util/BUILD @@ -259,6 +259,16 @@ cc_library( ], ) +cc_library( + name = "vector_tree_iterators", + hdrs = ["vector_tree_iterators.h"], + deps = [ + ":iterator_range", + ":logging", + ":vector_tree", + ], +) + cc_library( name = "map_tree", hdrs = ["map_tree.h"], @@ -491,6 +501,17 @@ cc_test( ], ) +cc_test( + name = "vector_tree_iterators_test", + srcs = ["vector_tree_iterators_test.cc"], + deps = [ + ":vector_tree_iterators", + ":vector_tree", + "@com_google_absl//absl/strings", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "map_tree_test", srcs = ["map_tree_test.cc"], diff --git a/common/util/vector_tree_iterators.h b/common/util/vector_tree_iterators.h new file mode 100644 index 000000000..1d9e53691 --- /dev/null +++ b/common/util/vector_tree_iterators.h @@ -0,0 +1,178 @@ +// 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. + +#ifndef VERIBLE_COMMON_UTIL_VECTOR_TREE_ITERATORS_H_ +#define VERIBLE_COMMON_UTIL_VECTOR_TREE_ITERATORS_H_ + +#include + +#include "common/util/iterator_range.h" +#include "common/util/logging.h" +#include "common/util/vector_tree.h" + +namespace verible { + +// Class implementing common VectorTree*Iterator members using CRTP polymorphic +// chaining. Derived class must implement following methods: +// - `static VectorTreeType* _NextNode(VectorTreeType* node)` - returns pointer +// to a next node +template +class _VectorTreeIteratorBase { + public: + using iterator_category = std::forward_iterator_tag; + using difference_type = std::ptrdiff_t; + using value_type = VectorTreeType; + using pointer = VectorTreeType*; + using reference = VectorTreeType&; + + _VectorTreeIteratorBase() : node_(nullptr) {} + _VectorTreeIteratorBase(pointer node) : node_(node) {} + + reference operator*() const { + CHECK_NOTNULL(node_); + return *node_; + } + pointer operator->() const { + CHECK_NOTNULL(node_); + return node_; + } + ImplType& operator++() { + node_ = ImplType::_NextNode(node_); + return static_cast(*this); + } + ImplType operator++(int) { + ImplType tmp = static_cast(*this); + node_ = ImplType::_NextNode(node_); + return tmp; + } + friend ImplType operator+(ImplType lhs, std::size_t rhs) { + while (rhs--) ++lhs; + return lhs; + } + friend bool operator==(const ImplType& a, const ImplType& b) { + return a.node_ == b.node_; + }; + friend bool operator!=(const ImplType& a, const ImplType& b) { + return a.node_ != b.node_; + }; + + protected: + pointer node_; +}; + +template +class VectorTreeLeavesIterator + : public _VectorTreeIteratorBase, + VectorTreeType> { + using this_type = VectorTreeLeavesIterator; + using base_type = _VectorTreeIteratorBase; + + public: + VectorTreeLeavesIterator() : base_type() {} + VectorTreeLeavesIterator(VectorTreeType* node) + : base_type(node ? node->LeftmostDescendant() : nullptr) {} + + static VectorTreeType* _NextNode(VectorTreeType* node) { + if (!node) return nullptr; + return node->NextLeaf(); + } +}; + +// Returns VectorTreeLeavesIterator range that spans all leaves of a tree. +// Note that when the node passed as a tree is a leaf, the returned range spans +// this node. +template +iterator_range> +VectorTreeLeavesTraversal(VectorTreeType& tree) { + VectorTreeLeavesIterator begin(tree.LeftmostDescendant()); + VectorTreeLeavesIterator end(tree.RightmostDescendant()); + ++end; + return {begin, end}; +} + +template +class VectorTreePreOrderIterator + : public _VectorTreeIteratorBase, + VectorTreeType> { + using this_type = VectorTreePreOrderIterator; + using base_type = _VectorTreeIteratorBase; + + public: + VectorTreePreOrderIterator() : base_type() {} + VectorTreePreOrderIterator(VectorTreeType* node) : base_type(node) {} + + static VectorTreeType* _NextNode(VectorTreeType* node) { + if (!node) return nullptr; + if (!node->is_leaf()) return &node->Children().front(); + while (node && node->IsLastChild()) { + node = node->Parent(); + } + if (node) node = node->NextSibling(); + return node; + } + + this_type begin() const { return *this; } + this_type end() const { + if (!this->node_) return this_type(nullptr); + return this_type(_NextNode(this->node_->RightmostDescendant())); + } +}; + +// Returns VectorTreePreOrderIterator range that spans all nodes of a tree +// (including the tree's root). +template +iterator_range> +VectorTreePreOrderTraversal(VectorTreeType& tree) { + VectorTreePreOrderIterator it(&tree); + return {it.begin(), it.end()}; +} + +template +class VectorTreePostOrderIterator + : public _VectorTreeIteratorBase< + VectorTreePostOrderIterator, VectorTreeType> { + using this_type = VectorTreePostOrderIterator; + using base_type = _VectorTreeIteratorBase; + + public: + VectorTreePostOrderIterator() : base_type() {} + VectorTreePostOrderIterator(VectorTreeType* node) : base_type(node) {} + + static VectorTreeType* _NextNode(VectorTreeType* node) { + if (!node) return nullptr; + if (node->IsLastChild()) return node->Parent(); + node = node->NextSibling(); + if (!node->is_leaf()) node = node->LeftmostDescendant(); + return node; + } + + this_type begin() const { + if (!this->node_) return this_type(nullptr); + return this_type(this->node_->LeftmostDescendant()); + } + this_type end() const { return this_type(_NextNode(this->node_)); } +}; + +// Returns VectorTreePostOrderIterator range that spans all nodes of a tree +// (including the tree's root). +template +iterator_range> +VectorTreePostOrderTraversal(VectorTreeType& tree) { + VectorTreePostOrderIterator it(&tree); + return {it.begin(), it.end()}; +} + +} // namespace verible + +#endif // VERIBLE_COMMON_UTIL_VECTOR_TREE_ITERATORS_H_ diff --git a/common/util/vector_tree_iterators_test.cc b/common/util/vector_tree_iterators_test.cc new file mode 100644 index 000000000..6d8c33328 --- /dev/null +++ b/common/util/vector_tree_iterators_test.cc @@ -0,0 +1,370 @@ +// 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. + +#include "common/util/vector_tree_iterators.h" + +#include +#include +#include +#include +#include + +#include "absl/strings/str_join.h" +#include "common/util/vector_tree.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace verible { + +// Iterator value printers used by Google Test + +template +static void PrintTo(const VectorTreeLeavesIterator& it, + std::ostream* os) { + *os << "VectorTreeLeavesIterator(" << *it << ")"; +} + +template +static void PrintTo(const VectorTreePreOrderIterator& it, + std::ostream* os) { + *os << "VectorTreePreOrderIterator(" << *it << ")"; +} + +template +static void PrintTo(const VectorTreePostOrderIterator& it, + std::ostream* os) { + *os << "VectorTreePostOrderIterator(" << *it << ")"; +} + +namespace { + +using Tree = verible::VectorTree; + +template +void ExpectForwardIterator(TreeType* node, TreeType* next_node) { + EXPECT_TRUE(std::is_default_constructible_v); + EXPECT_TRUE(std::is_copy_constructible_v); + EXPECT_TRUE(std::is_copy_assignable_v); + EXPECT_TRUE(std::is_destructible_v); + EXPECT_TRUE(std::is_swappable_v); + + Iterator node_it(node), next_node_it(next_node); + Iterator node_it_2(node); + // i == j + EXPECT_TRUE(node_it == node_it_2); + // i != j + EXPECT_TRUE(node_it != next_node_it); + // *i + EXPECT_EQ(&(*node_it), node); + EXPECT_EQ(&(*next_node_it), next_node); + // i->member + EXPECT_EQ(next_node_it->Value(), next_node->Value()); + // ++i + { + auto i = node_it; + EXPECT_EQ(++i, next_node_it); + EXPECT_EQ(i, next_node_it); + EXPECT_NE(i, node_it); + } + // i++ + { + auto i = node_it; + EXPECT_EQ(i++, node_it); + EXPECT_EQ(i, next_node_it); + EXPECT_NE(i, node_it); + } + // *i++ + { + auto i = node_it; + EXPECT_EQ(&(*i++), node); + EXPECT_EQ(i, next_node_it); + EXPECT_NE(i, node_it); + } + + if constexpr (!std::is_const_v) { + auto tree = Tree(0, // + Tree(1), // + Tree(2)); + auto other_tree = Tree(200, // + Tree(2001), // + Tree(2002)); + auto it = Iterator(&tree.Children()[1]); + // *i = j + *it = other_tree; + EXPECT_EQ(tree.Children()[1].Value(), 200); + } +} + +TEST(VectorTreeIteratorTest, ForwardIteratorInterface) { + auto tree = Tree(0, // + Tree(1), // + Tree(2, // + Tree(21), // + Tree(22), // + Tree(23)), // + Tree(3)); + + ExpectForwardIterator>( + &tree.Children()[0], &tree.Children()[1].Children()[0]); + ExpectForwardIterator>( + &tree.Children()[0], &tree.Children()[1].Children()[0]); + + ExpectForwardIterator>(&tree.Children()[0], + &tree.Children()[1]); + ExpectForwardIterator>( + &tree.Children()[0], &tree.Children()[1]); + + ExpectForwardIterator>( + &tree.Children()[0], &tree.Children()[1].Children()[0]); + ExpectForwardIterator>( + &tree.Children()[0], &tree.Children()[1].Children()[0]); +} + +struct TestCaseData { + Tree tree; + + // RootNodeTraversal test data + struct { + std::vector expected_sequence_leaves; + std::vector expected_sequence_pre_order; + std::vector expected_sequence_post_order; + } root_node_traversal; + + // SubtreeTraversal and IteratorSubtreeTraversal test data + struct { + std::vector subtree_path; + std::vector expected_sequence_leaves; + std::vector expected_sequence_pre_order; + std::vector expected_sequence_post_order; + } subtree_traversal; +}; + +static const TestCaseData kTestCasesData[] = { + { + Tree(0), + // RootNodeTraversal test data + { + {0}, + {0}, + {0}, + }, + // SubtreeTraversal and IteratorSubtreeTraversal test data (skipped) + {}, + }, + { + Tree(0, // + Tree(1), // + Tree(2), // + Tree(3)), + // RootNodeTraversal test data + { + {1, 2, 3}, + {0, 1, 2, 3}, + {1, 2, 3, 0}, + }, + // SubtreeTraversal and IteratorSubtreeTraversal test data + { + {0}, // subtree path + {1}, + {1}, + {1}, + }, + }, + { + Tree(0, // + Tree(1, // + Tree(11, // + Tree(111, // + Tree(1111))))), + // RootNodeTraversal test data + { + {1111}, + {0, 1, 11, 111, 1111}, + {1111, 111, 11, 1, 0}, + }, + // SubtreeTraversal and IteratorSubtreeTraversal test data + { + {0, 0}, // subtree path + {1111}, + {11, 111, 1111}, + {1111, 111, 11}, + }, + }, + { + Tree(0, // + Tree(1, // + Tree(11, // + Tree(111), // + Tree(112)), // + Tree(12), // + Tree(13)), // + Tree(2, // + Tree(21), // + Tree(22), // + Tree(23, // + Tree(231), // + Tree(232))), // + Tree(3)), + // RootNodeTraversal test data + { + {111, 112, 12, 13, 21, 22, 231, 232, 3}, + {0, 1, 11, 111, 112, 12, 13, 2, 21, 22, 23, 231, 232, 3}, + {111, 112, 11, 12, 13, 1, 21, 22, 231, 232, 23, 2, 3, 0}, + }, + // SubtreeTraversal and IteratorSubtreeTraversal test data + { + {0}, // subtree path + {111, 112, 12, 13}, + {1, 11, 111, 112, 12, 13}, + {111, 112, 11, 12, 13, 1}, + }, + }, + { + Tree(0, // + Tree(1), // + Tree(2, // + Tree(21, // + Tree(211), // + Tree(212)), // + Tree(22), // + Tree(23)), // + Tree(3, // + Tree(31), // + Tree(32), // + Tree(33, // + Tree(331), // + Tree(332)))), + // RootNodeTraversal test data + { + {1, 211, 212, 22, 23, 31, 32, 331, 332}, + {0, 1, 2, 21, 211, 212, 22, 23, 3, 31, 32, 33, 331, 332}, + {1, 211, 212, 21, 22, 23, 2, 31, 32, 331, 332, 33, 3, 0}, + }, + // SubtreeTraversal and IteratorSubtreeTraversal test data + { + {2}, // subtree path + {31, 32, 331, 332}, + {3, 31, 32, 33, 331, 332}, + {31, 32, 331, 332, 33, 3}, + }, + }, +}; + +template +void ExpectNodesRangesValuesEq(const NodesRange& nodes, + const ValuesRange& expected_values) { + auto expected_it = expected_values.begin(); + + for (const auto& node : nodes) { + EXPECT_NE(expected_it, expected_values.end()); + EXPECT_EQ(node.Value(), *expected_it); + ++expected_it; + } + EXPECT_EQ(expected_it, expected_values.end()); +} + +TEST(VectorTreeIteratorTest, RootNodeTraversal) { + for (const auto& data : kTestCasesData) { + std::ostringstream trace_msg; + trace_msg << "Input tree:\n" << data.tree; + SCOPED_TRACE(trace_msg.str()); + + { + SCOPED_TRACE("VectorTreeLeavesTraversal"); + ExpectNodesRangesValuesEq( + VectorTreeLeavesTraversal(data.tree), + data.root_node_traversal.expected_sequence_leaves); + } + { + SCOPED_TRACE("VectorTreePreOrderTraversal"); + ExpectNodesRangesValuesEq( + VectorTreePreOrderTraversal(data.tree), + data.root_node_traversal.expected_sequence_pre_order); + } + { + SCOPED_TRACE("VectorTreePostOrderTraversal"); + ExpectNodesRangesValuesEq( + VectorTreePostOrderTraversal(data.tree), + data.root_node_traversal.expected_sequence_post_order); + } + } +} + +TEST(VectorTreeIteratorTest, SubtreeTraversal) { + for (const auto& data : kTestCasesData) { + const auto subtree_path = data.subtree_traversal.subtree_path; + if (subtree_path.empty()) continue; + + const auto& subtree = + data.tree.DescendPath(subtree_path.begin(), subtree_path.end()); + + std::ostringstream trace_msg; + trace_msg << "Input tree:\n" + << data.tree + << "\nSubtree path: " << absl::StrJoin(subtree_path, "."); + SCOPED_TRACE(trace_msg.str()); + + { + SCOPED_TRACE("VectorTreeLeavesTraversal"); + ExpectNodesRangesValuesEq( + VectorTreeLeavesTraversal(subtree), + data.subtree_traversal.expected_sequence_leaves); + } + { + SCOPED_TRACE("VectorTreePreOrderTraversal"); + ExpectNodesRangesValuesEq( + VectorTreePreOrderTraversal(subtree), + data.subtree_traversal.expected_sequence_pre_order); + } + { + SCOPED_TRACE("VectorTreePostOrderTraversal"); + ExpectNodesRangesValuesEq( + VectorTreePostOrderTraversal(subtree), + data.subtree_traversal.expected_sequence_post_order); + } + } +} + +TEST(VectorTreeIteratorTest, IteratorSubtreeTraversal) { + for (const auto& data : kTestCasesData) { + const auto subtree_path = data.subtree_traversal.subtree_path; + if (subtree_path.empty()) continue; + + const auto& subtree = + data.tree.DescendPath(subtree_path.begin(), subtree_path.end()); + + std::ostringstream trace_msg; + trace_msg << "Input tree:\n" + << data.tree + << "\nSubtree path: " << absl::StrJoin(subtree_path, "."); + SCOPED_TRACE(trace_msg.str()); + + // VectorTreeLeavesIterator doesn't support subranges + { + SCOPED_TRACE("VectorTreePreOrderIterator"); + ExpectNodesRangesValuesEq( + VectorTreePreOrderIterator(&subtree), + data.subtree_traversal.expected_sequence_pre_order); + } + { + SCOPED_TRACE("VectorTreePostOrderIterator"); + ExpectNodesRangesValuesEq( + VectorTreePostOrderIterator(&subtree), + data.subtree_traversal.expected_sequence_post_order); + } + } +} + +} // namespace +} // namespace verible From 06d41e07e494d10598ac25bac9d6abffd6d3c0ca Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Tue, 22 Jun 2021 18:24:50 +0200 Subject: [PATCH 02/23] Add support for subcolumns in tabular alignment mode --- common/formatting/BUILD | 4 + common/formatting/align.cc | 712 +++++++++++++++++++++++--------- common/formatting/align.h | 64 ++- common/formatting/align_test.cc | 347 ++++++++++++++++ common/util/vector_tree.h | 5 +- verilog/formatting/align.cc | 10 +- 6 files changed, 922 insertions(+), 220 deletions(-) diff --git a/common/formatting/BUILD b/common/formatting/BUILD index cf25c45b8..ff359565f 100644 --- a/common/formatting/BUILD +++ b/common/formatting/BUILD @@ -28,7 +28,10 @@ cc_library( "//common/util:algorithm", "//common/util:container_iterator_range", "//common/util:enum_flags", + "//common/util:iterator_range", "//common/util:logging", + "//common/util:vector_tree", + "//common/util:vector_tree_iterators", "@com_google_absl//absl/strings", ], ) @@ -44,6 +47,7 @@ cc_test( "//common/text:tree_builder_test_util", "//common/util:range", "//common/util:spacer", + "//common/util:value_saver", "@com_google_absl//absl/strings", "@com_google_googletest//:gtest_main", ], diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 653d0aa84..bfa7faea5 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -36,7 +36,10 @@ #include "common/util/algorithm.h" #include "common/util/container_iterator_range.h" #include "common/util/enum_flags.h" +#include "common/util/iterator_range.h" #include "common/util/logging.h" +#include "common/util/vector_tree.h" +#include "common/util/vector_tree_iterators.h" namespace verible { @@ -85,6 +88,8 @@ static int EffectiveLeftBorderWidth(const MutableFormatTokenRange& tokens) { return tokens.front().before.spaces_required; } +using ColumnsTreePath = SyntaxTreePath; + struct AlignmentCell { // Slice of format tokens in this cell (may be empty range). MutableFormatTokenRange tokens; @@ -94,6 +99,11 @@ struct AlignmentCell { // as a space-only column, usually no more than 1 space wide. int left_border_width = 0; + // Returns true when neither the cell nor its subcells contain any tokens. + bool IsUnused() const { return (tokens.empty() && compact_width == 0); } + // Returns true when the cell contains subcells with tokens. + bool IsComposite() const { return (tokens.empty() && compact_width > 0); } + int TotalWidth() const { return left_border_width + compact_width; } FormatTokenRange ConstTokensRange() const { @@ -106,6 +116,9 @@ struct AlignmentCell { } }; +using AlignmentRow = VectorTree; +using AlignmentMatrix = std::vector; + std::ostream& operator<<(std::ostream& stream, const AlignmentCell& cell) { if (!cell.tokens.empty()) { // See UnwrappedLine::AsCode for similar printing. @@ -117,6 +130,133 @@ std::ostream& operator<<(std::ostream& stream, const AlignmentCell& cell) { return stream; } +template +static std::size_t CreateTextNodes( + const VectorTree& src_node, VectorTree* dst_node, + const std::function( + const VectorTree&)>& get_cell_label) { + static constexpr std::size_t kMinCellWidth = 2; + + std::size_t depth = 0; + std::size_t subtree_depth = 0; + + for (const auto& src_child : src_node.Children()) { + const auto [text, filler] = get_cell_label(src_child); + const std::vector lines = absl::StrSplit(text, '\n'); + auto* dst_child = dst_node; + for (const auto line : lines) { + dst_child = dst_child->NewChild( + Cell{line, filler, std::max(line.size(), kMinCellWidth)}); + } + depth = std::max(depth, lines.size()); + subtree_depth = std::max( + subtree_depth, CreateTextNodes(src_child, dst_child, get_cell_label)); + } + return depth + subtree_depth; +} + +template +static void ColumnsTreeFormatter( + std::ostream& stream, const VectorTree& root, + const std::function( + const VectorTree&)>& get_cell_label) { + if (root.Children().empty()) return; + + static constexpr absl::string_view kCellSeparator = "|"; + + struct Cell { + std::string text = ""; + char filler = ' '; + std::size_t width = 0; + }; + VectorTree text_tree; + + const std::size_t depth = + CreateTextNodes(root, &text_tree, get_cell_label); + + // Adjust cells width to fit all their children + for (auto& node : VectorTreePostOrderTraversal(text_tree)) { + // Include separator width in cell width + node.Value().width += kCellSeparator.size(); + if (node.is_leaf()) continue; + std::size_t children_width = + std::accumulate(node.Children().begin(), node.Children().end(), 0, + [](std::size_t width, const VectorTree& child) { + return width + child.Value().width; + }); + if (node.Value().width < children_width) { + node.Value().width = children_width; + } + } + // Adjust cells width to fill their parents + for (auto& node : VectorTreePreOrderTraversal(text_tree)) { + if (node.is_leaf()) continue; + std::size_t children_width = + std::accumulate(node.Children().begin(), node.Children().end(), 0, + [](std::size_t width, const VectorTree& child) { + return width + child.Value().width; + }); + if (node.Value().width > children_width) { + auto extra_width = node.Value().width - children_width; + for (auto& child : node.Children()) { + const auto added_child_width = + extra_width * child.Value().width / children_width; + extra_width -= added_child_width; + children_width -= child.Value().width; + child.Value().width += added_child_width; + } + } + } + + std::vector lines(depth); + auto range = VectorTreePreOrderTraversal(text_tree); + auto level_offset = text_tree.NumAncestors() + 1; + for (auto& node : make_range(range.begin() + 1, range.end())) { + auto& cell = node.Value(); + const std::size_t level = node.NumAncestors() - level_offset; + if (level > 0 && node.IsFirstChild()) { + const int padding_len = lines[level - 1].size() - lines[level].size() - + node.Parent()->Value().width; + if (padding_len > 0) { + if (lines[level].empty()) + lines[level].append(std::string(padding_len, ' ')); + else if (padding_len > int(kCellSeparator.size())) + lines[level].append(absl::StrCat( + kCellSeparator, + std::string(padding_len - kCellSeparator.size(), ' '))); + } + } + + const std::vector parts = + absl::StrSplit(cell.text, '\t'); + CHECK_LE(parts.size(), 3); + + const auto width = cell.width - kCellSeparator.size(); + + if (parts.size() == 1) { + const std::string pad(width - parts[0].size(), cell.filler); + absl::StrAppend(&lines[level], kCellSeparator, parts[0], pad); + } else if (parts.size() == 2) { + const std::string pad(width - parts[0].size() - parts[1].size(), + cell.filler); + absl::StrAppend(&lines[level], kCellSeparator, parts[0], pad, + parts.back()); + } else if (parts.size() == 3) { + std::size_t pos = + std::clamp((width - parts[1].size()) / 2, parts[0].size() + 1, + width - parts[2].size() - parts[1].size() - 1); + const std::string left_pad(pos - parts[0].size(), cell.filler); + const std::string right_pad( + width - parts[2].size() - (pos + parts[1].size()), cell.filler); + absl::StrAppend(&lines[level], kCellSeparator, parts[0], left_pad, + parts[1], right_pad, parts[2]); + } + } + for (const auto& line : lines) { + if (!line.empty()) stream << line << kCellSeparator << "\n"; + } +} + // These properties are calculated/aggregated from alignment cells. struct AlignedColumnConfiguration { int width = 0; @@ -130,38 +270,50 @@ struct AlignedColumnConfiguration { } }; -typedef std::vector AlignmentRow; -typedef std::vector AlignmentMatrix; - -void ColumnSchemaScanner::ReserveNewColumn( - const Symbol& symbol, const AlignmentColumnProperties& properties, - const SyntaxTreePath& path) { +ColumnPositionTree* ColumnSchemaScanner::ReserveNewColumn( + ColumnPositionTree& parent_column, const Symbol& symbol, + const AlignmentColumnProperties& properties, const SyntaxTreePath& path) { // The path helps establish a total ordering among all desired alignment // points, given that they may come from optional or repeated language // constructs. const SyntaxTreeLeaf* leaf = GetLeftmostLeaf(symbol); // It is possible for a node to be empty, in which case, ignore. - if (leaf == nullptr) return; - if (sparse_columns_.empty() || sparse_columns_.back().path != path) { - // It's possible the previous cell's path was intentionally altered - // to effectively fuse it with the cell that is about to be added. - // When this occurs, take the (previous) leftmost token, and suppress - // adding a new column. - sparse_columns_.push_back( + if (leaf == nullptr) return nullptr; + if (parent_column.Parent() != nullptr && parent_column.Children().empty()) { + // Starting token of a column and its first subcolumn must be the same. + // (subcolumns overlap their parent column). + CHECK_EQ(parent_column.Value().starting_token, leaf->get()); + } + // It's possible the previous cell's path was intentionally altered + // to effectively fuse it with the cell that is about to be added. + // When this occurs, take the (previous) leftmost token, and suppress + // adding a new column. + if (parent_column.Children().empty() || + parent_column.Children().back().Value().path != path) { + const auto* column = parent_column.NewChild( ColumnPositionEntry{path, leaf->get(), properties}); - VLOG(2) << "reserving new column at " << TreePathFormatter(path); + ColumnsTreePath column_path; + column->Path(column_path); + VLOG(2) << "reserving new column for " << TreePathFormatter(path) << " at " + << TreePathFormatter(column_path); } + return &parent_column.Children().back(); } struct AggregateColumnData { + AggregateColumnData() = default; + // This is taken as the first seen set of properties in any given column. AlignmentColumnProperties properties; // These tokens's positions will be used to identify alignment cell // boundaries. std::vector starting_tokens; + SyntaxTreePath path; + void Import(const ColumnPositionEntry& cell) { if (starting_tokens.empty()) { + path = cell.path; // Take the first set of properties, and ignore the rest. // They should be consistent, coming from alignment cell scanners, // but this is not verified. @@ -171,53 +323,165 @@ struct AggregateColumnData { } }; +template +static void CopyTreeStructure( + const VectorTree& src_tree, + VectorTree* dst_tree, + const std::function& converter) { + for (const auto& src_child : src_tree.Children()) { + auto* dst_child = dst_tree->NewChild(converter(src_child.Value())); + CopyTreeStructure(src_child, dst_child, converter); + } +} + class ColumnSchemaAggregator { public: - void Collect(const std::vector& row) { - for (const auto& cell : row) { - cell_map_[cell.path].Import(cell); - } + void Collect(const ColumnPositionTree& columns) { + CollectColumnsTree(columns, columns_); } - size_t NumUniqueColumns() const { return cell_map_.size(); } - - // Establishes 1:1 between SyntaxTreePath and column index. - // Call this after collecting all columns. - void FinalizeColumnIndices() { - column_positions_.reserve(cell_map_.size()); - for (const auto& kv : cell_map_) { - column_positions_.push_back(kv.first); + // Sort columns by syntax tree path assigned to them and create an index that + // maps syntax tree path to a column. Call this after collecting all columns. + void Finalize() { + syntax_to_columns_map_.clear(); + + for (auto& node : VectorTreePreOrderTraversal(columns_)) { + if (node.Parent()) { + // Index the column + auto it = syntax_to_columns_map_.emplace_hint( + syntax_to_columns_map_.end(), node.Value().path, ColumnsTreePath{}); + node.Path(it->second); + } + if (!node.is_leaf()) { + // Sort subcolumns + std::sort(node.Children().begin(), node.Children().end(), + [](const auto& a, const auto& b) { + return a.Value().path < b.Value().path; + }); + } } } - const std::vector& ColumnPositions() const { - return column_positions_; + const std::map& SyntaxToColumnsMap() const { + return syntax_to_columns_map_; } - std::vector ColumnProperties() const { - std::vector properties; - properties.reserve(cell_map_.size()); - for (const auto& entry : cell_map_) { - properties.push_back(entry.second.properties); - } + const VectorTree& Columns() const { return columns_; } + + VectorTree ColumnProperties() const { + VectorTree properties; + CopyTreeStructure( + columns_, &properties, + [](const AggregateColumnData& data) { return data.properties; }); return properties; } private: + void CollectColumnsTree(const ColumnPositionTree& column, + VectorTree& aggregate_column) { + for (const auto& subcolumn : column.Children()) { + const auto [index_entry, insert] = + syntax_to_columns_map_.try_emplace(subcolumn.Value().path); + VectorTree* aggregate_subcolumn; + if (insert) { + aggregate_subcolumn = aggregate_column.NewChild(); + CHECK_NOTNULL(aggregate_subcolumn); + // Put aggregate column node's path in created index entry + aggregate_subcolumn->Path(index_entry->second); + } else { + // Fact: existing aggregate_subcolumn is a direct child of + // aggregate_column + CHECK_GT(aggregate_column.Children().size(), + index_entry->second.back()); + aggregate_subcolumn = + &aggregate_column.Children()[index_entry->second.back()]; + } + aggregate_subcolumn->Value().Import(subcolumn.Value()); + CollectColumnsTree(subcolumn, *aggregate_subcolumn); + } + } + // Keeps track of unique positions where new columns are desired. - // The keys form the set of columns wanted across all rows. - // The values are sets of starting tokens, from which token ranges - // will be computed per cell. - std::map cell_map_; - - // 1:1 map between SyntaxTreePath and column index. - // Values are monotonically increasing, so this is binary_search-able. - std::vector column_positions_; + // The nodes are sets of starting tokens, from which token ranges will be + // computed per cell. + VectorTree columns_; + // 1:1 map between syntax tree's path and columns tree's path + std::map syntax_to_columns_map_; }; -static SequenceStreamFormatter MatrixRowFormatter( - const AlignmentRow& row) { - return SequenceFormatter(row, " | ", "< ", " >"); +template +static std::pair GetColumnDataCellLabel( + const VectorTree& node) { + const absl::string_view arrow = + node.Value().properties.flush_left ? "<" : ">"; + + std::ostringstream label; + const auto& path = node.Value().path; + auto begin = path.begin(); + if (node.Parent()) { + // Find and skip common prefix + const auto& parent_path = node.Parent()->Value().path; + auto parent_begin = parent_path.begin(); + while (begin != path.end() && parent_begin != parent_path.end() && + *begin == *parent_begin) { + ++begin; + ++parent_begin; + } + } + label << " " << arrow << "\t"; + if (begin != path.begin() && begin != path.end()) label << "."; + label << SequenceFormatter(iterator_range(begin, path.end()), "."); + label << "\t" << arrow << " "; + + return {label.str(), '-'}; +} + +std::ostream& operator<<(std::ostream& stream, + const VectorTree& tree) { + ColumnsTreeFormatter( + stream, tree, GetColumnDataCellLabel); + return stream; +} + +std::ostream& operator<<(std::ostream& stream, const ColumnPositionTree& tree) { + ColumnsTreeFormatter( + stream, tree, GetColumnDataCellLabel); + return stream; +} + +std::ostream& operator<<(std::ostream& stream, + const VectorTree& tree) { + ColumnsTreeFormatter( + stream, tree, + [](const VectorTree& node) + -> std::pair { + const auto& cell = node.Value(); + if (cell.IsUnused()) { + return {"", '.'}; + } else { + const auto width_info = absl::StrCat("\t(", cell.left_border_width, + "+", cell.compact_width, ")\t"); + if (cell.IsComposite()) + return {absl::StrCat("/", width_info, "\\"), '`'}; + + std::ostringstream label; + label << "\t" << cell << "\t\n" << width_info; + return {label.str(), ' '}; + } + }); + return stream; +} + +std::ostream& operator<<(std::ostream& stream, + const VectorTree& tree) { + ColumnsTreeFormatter( + stream, tree, [](const VectorTree& node) { + const auto& cell = node.Value(); + const auto label = + absl::StrCat("\t", cell.left_border, "+", cell.width, "\t"); + return std::pair{label, ' '}; + }); + return stream; } struct AlignmentRowData { @@ -226,109 +490,85 @@ struct AlignmentRowData { // Set of cells found that correspond to an ordered, sparse set of columns // to be aligned with other rows. - std::vector sparse_columns; + ColumnPositionTree sparse_columns; }; -// Translate a sparse set of columns into a fully-populated matrix row. static void FillAlignmentRow( const AlignmentRowData& row_data, - const std::vector& column_positions, AlignmentRow* row) { - VLOG(2) << __FUNCTION__; + const std::map& columns_map, + AlignmentRow* row) { const auto& sparse_columns(row_data.sparse_columns); - MutableFormatTokenRange partition_token_range(row_data.ftoken_range); - // Translate token into preformat_token iterator, - // full token range. - const auto cbegin = column_positions.begin(); - const auto cend = column_positions.end(); - auto pos_iter = cbegin; - auto token_iter = partition_token_range.begin(); - const auto token_end = partition_token_range.end(); - int last_column_index = 0; - // Find each non-empty cell, and fill in other cells with empty ranges. - for (const auto& col : sparse_columns) { - pos_iter = std::find(pos_iter, cend, col.path); - // By construction, sparse_columns' paths should be a subset of those - // in the aggregated column_positions set. - CHECK(pos_iter != cend); - const int column_index = std::distance(cbegin, pos_iter); - VLOG(3) << "cell at column " << column_index; - - // Find the format token iterator that corresponds to the column start. - // Linear time total over all outer loop iterations. - token_iter = - std::find_if(token_iter, token_end, [=](const PreFormatToken& ftoken) { - return BoundsEqual(ftoken.Text(), col.starting_token.text()); - }); - CHECK(token_iter != token_end); - - // Fill null-range cells between [last_column_index, column_index). - const MutableFormatTokenRange empty_filler(token_iter, token_iter); - for (; last_column_index <= column_index; ++last_column_index) { - VLOG(3) << "empty at column " << last_column_index; - (*row)[last_column_index].tokens = empty_filler; + MutableFormatTokenRange remaining_tokens_range(row_data.ftoken_range); + + MutableFormatTokenRange* prev_cell_tokens = nullptr; + if (!sparse_columns.is_leaf()) { + for (const auto& col : VectorTreeLeavesTraversal(sparse_columns)) { + const auto column_loc_iter = columns_map.find(col.Value().path); + CHECK(column_loc_iter != columns_map.end()); + + const auto token_iter = std::find_if( + remaining_tokens_range.begin(), remaining_tokens_range.end(), + [=](const PreFormatToken& ftoken) { + return BoundsEqual(ftoken.Text(), + col.Value().starting_token.text()); + }); + CHECK(token_iter != remaining_tokens_range.end()); + remaining_tokens_range.set_begin(token_iter); + + if (prev_cell_tokens != nullptr) prev_cell_tokens->set_end(token_iter); + + AlignmentRow& row_cell = row->DescendPath(column_loc_iter->second.begin(), + column_loc_iter->second.end()); + row_cell.Value().tokens = remaining_tokens_range; + prev_cell_tokens = &row_cell.Value().tokens; } - // At this point, the current cell has only seen its lower bound. - // The upper bound will be set in a separate pass. } - // Fill any sparse cells up to the last column. - VLOG(3) << "fill up to last column"; - const MutableFormatTokenRange empty_filler(token_end, token_end); - for (const int n = column_positions.size(); last_column_index < n; - ++last_column_index) { - VLOG(3) << "empty at column " << last_column_index; - (*row)[last_column_index].tokens = empty_filler; - } - - // In this pass, set the upper bounds of cells' token ranges. - auto upper_bound = token_end; - for (auto& cell : verible::reversed_view(*row)) { - cell.tokens.set_end(upper_bound); - upper_bound = cell.tokens.begin(); - } - VLOG(2) << "end of " << __FUNCTION__ << ", row: " << MatrixRowFormatter(*row); } -struct MatrixCellSizeFormatter { - const AlignmentMatrix& matrix; -}; +static void UpdateAndPropagateRowCellWidths(AlignmentRow* node) { + node->Value().UpdateWidths(); -std::ostream& operator<<(std::ostream& stream, - const MatrixCellSizeFormatter& p) { - const AlignmentMatrix& matrix = p.matrix; - for (const auto& row : matrix) { - stream << '[' - << absl::StrJoin(row, ", ", - [](std::string* out, const AlignmentCell& cell) { - absl::StrAppend(out, cell.left_border_width, "+", - cell.compact_width); - }) - << ']' << std::endl; + if (node->is_leaf()) return; + + int total_width = 0; + for (auto& child : node->Children()) { + UpdateAndPropagateRowCellWidths(&child); + total_width += child.Value().TotalWidth(); + } + + if (node->Value().tokens.empty()) { + node->Value().left_border_width = + node->Children().front().Value().left_border_width; + node->Value().compact_width = total_width - node->Value().left_border_width; } - return stream; } -static void ComputeCellWidths(AlignmentMatrix* matrix) { +static void ComputeRowCellWidths(AlignmentRow* row) { VLOG(2) << __FUNCTION__; - for (auto& row : *matrix) { - for (auto& cell : row) { - cell.UpdateWidths(); - } - // Force leftmost table border to be 0 because these cells start new lines - // and thus should not factor into alignment calculation. - // Note: this is different from how StateNode calculates column positions. - row.front().left_border_width = 0; + UpdateAndPropagateRowCellWidths(row); + + // Force leftmost table border to be 0 because these cells start new lines + // and thus should not factor into alignment calculation. + // Note: this is different from how StateNode calculates column positions. + auto* front = row; + while (!front->Children().empty()) { + front = &front->Children().front(); + front->Value().left_border_width = 0; } - VLOG(2) << "end of " << __FUNCTION__ << ", cell sizes:\n" - << MatrixCellSizeFormatter{*matrix}; + VLOG(2) << "end of " << __FUNCTION__; } -typedef std::vector AlignedFormattingColumnSchema; +using AlignedFormattingColumnSchema = VectorTree; static AlignedFormattingColumnSchema ComputeColumnWidths( const AlignmentMatrix& matrix, - const std::vector& column_properties) { + const VectorTree& column_properties) { VLOG(2) << __FUNCTION__; - AlignedFormattingColumnSchema column_configs(matrix.front().size()); + + AlignedFormattingColumnSchema column_configs; + CopyTreeStructure( + matrix.front(), &column_configs, + [](const AlignmentCell&) { return AlignedColumnConfiguration{}; }); // Check which cell before delimiter is the longest // If this cell is in the last row, the sizes of column with delimiter @@ -336,13 +576,16 @@ static AlignedFormattingColumnSchema ComputeColumnWidths( int longest_cell_before_delimiter = 0; bool align_to_last_row = false; for (const AlignmentRow& row : matrix) { - auto column_prop_iter = column_properties.begin(); - for (const AlignmentCell& cell : row) { + auto column_prop_iter = + VectorTreePreOrderTraversal(column_properties).begin(); + const auto column_prop_end = + VectorTreePreOrderTraversal(column_properties).end(); + for (const auto& node : VectorTreePreOrderTraversal(row)) { const auto next_prop = std::next(column_prop_iter, 1); - if (next_prop != column_properties.end() && - next_prop->contains_delimiter) { - if (longest_cell_before_delimiter < cell.TotalWidth()) { - longest_cell_before_delimiter = cell.TotalWidth(); + if (next_prop != column_prop_end && + next_prop->Value().contains_delimiter) { + if (longest_cell_before_delimiter < node.Value().TotalWidth()) { + longest_cell_before_delimiter = node.Value().TotalWidth(); if (&row == &matrix.back()) align_to_last_row = true; } break; @@ -352,19 +595,37 @@ static AlignedFormattingColumnSchema ComputeColumnWidths( } for (const AlignmentRow& row : matrix) { - auto column_iter = column_configs.begin(); - auto column_prop_iter = column_properties.begin(); - for (const AlignmentCell& cell : row) { - if (column_prop_iter->contains_delimiter && align_to_last_row) { - column_iter->width = 0; - column_iter->left_border = 0; + auto column_iter = VectorTreePreOrderTraversal(column_configs).begin(); + auto column_prop_iter = + VectorTreePreOrderTraversal(column_properties).begin(); + + for (const auto& node : VectorTreePreOrderTraversal(row)) { + if (column_prop_iter->Value().contains_delimiter && align_to_last_row) { + column_iter->Value().width = 0; + column_iter->Value().left_border = 0; } else { - column_iter->UpdateFromCell(cell); + column_iter->Value().UpdateFromCell(node.Value()); } ++column_iter; ++column_prop_iter; } } + + for (auto& cell : VectorTreePostOrderTraversal(column_configs)) { + if (!cell.is_leaf()) { + int children_width = std::accumulate( + cell.Children().begin(), cell.Children().end(), 0, + [](int width, const AlignedFormattingColumnSchema& node) { + return width + node.Value().TotalWidth(); + }); + cell.Value().left_border = + std::max(cell.Value().left_border, + cell.Children().front().Value().left_border); + cell.Value().width = std::max(cell.Value().width, + children_width - cell.Value().left_border); + } + } + VLOG(2) << "end of " << __FUNCTION__; return column_configs; } @@ -394,47 +655,97 @@ struct DeferredTokenAlignment { } }; -// Align cells by adjusting pre-token spacing for a single row. -static std::vector ComputeAlignedRowSpacings( - const AlignedFormattingColumnSchema& column_configs, - const std::vector& properties, - const AlignmentRow& row) { - VLOG(2) << __FUNCTION__; - std::vector align_actions; - int accrued_spaces = 0; - auto column_iter = column_configs.begin(); - auto properties_iter = properties.begin(); - for (const auto& cell : row) { - accrued_spaces += column_iter->left_border; - if (cell.tokens.empty()) { - // Accumulate spacing for the next sparse cell in this row. - accrued_spaces += column_iter->width; +static void ComputeAlignedRowCellSpacings( + const VectorTree& column_configs, + const VectorTree& properties, + const AlignmentRow& row, std::vector* align_actions, + int* accrued_spaces) { + ColumnsTreePath node_path; + row.Path(node_path); + VLOG(2) << TreePathFormatter(node_path) << " " << __FUNCTION__ << std::endl; + + if (row.Children().empty()) return; + + auto column_config_it = column_configs.Children().begin(); + auto column_properties_it = properties.Children().begin(); + for (const auto& cell : row.Children()) { + node_path.clear(); + cell.Path(node_path); + if (cell.Value().IsUnused()) { + const int total_width = column_config_it->Value().left_border + + column_config_it->Value().width; + + VLOG(2) << TreePathFormatter(node_path) + << " unused cell; width: " << total_width; + + *accrued_spaces += total_width; + } else if (cell.Value().IsComposite()) { + // Cummulative subcolumns width might be smaller than their parent + // column's width. + const int subcolumns_width = std::accumulate( + column_config_it->Children().begin(), + column_config_it->Children().end(), 0, + [](int width, const VectorTree& node) { + return width + node.Value().TotalWidth(); + }); + const int padding = + column_config_it->Value().TotalWidth() - subcolumns_width; + + VLOG(2) << TreePathFormatter(node_path) << " composite cell" + << "; padding: " << padding << "; flush: " + << (column_properties_it->Value().flush_left ? "left" : "right"); + + if (!column_properties_it->Value().flush_left) *accrued_spaces += padding; + ComputeAlignedRowCellSpacings(*column_config_it, *column_properties_it, + cell, align_actions, accrued_spaces); + if (column_properties_it->Value().flush_left) *accrued_spaces += padding; } else { - VLOG(2) << "at: " << cell.tokens.front().Text(); + *accrued_spaces += column_config_it->Value().left_border; + + VLOG(2) << TreePathFormatter(node_path) << " token cell" + << "; starting token: " << cell.Value().tokens.front().Text(); + // Align by setting the left-spacing based on sum of cell widths // before this one. - const int padding = column_iter->width - cell.compact_width; - PreFormatToken& ftoken = cell.tokens.front(); + const int padding = + column_config_it->Value().width - cell.Value().compact_width; + PreFormatToken& ftoken = cell.Value().tokens.front(); int left_spacing; - if (properties_iter->flush_left) { - if (properties_iter->contains_delimiter) { + if (column_properties_it->Value().flush_left) { + if (column_properties_it->Value().contains_delimiter) { left_spacing = 0; - accrued_spaces += padding; + *accrued_spaces += padding; } else { - left_spacing = accrued_spaces; - accrued_spaces = padding; + left_spacing = *accrued_spaces; + *accrued_spaces = padding; } } else { // flush right - left_spacing = accrued_spaces + padding; - accrued_spaces = 0; + left_spacing = *accrued_spaces + padding; + *accrued_spaces = 0; } - align_actions.emplace_back(&ftoken, left_spacing); - VLOG(2) << "left_spacing = " << left_spacing; + align_actions->emplace_back(&ftoken, left_spacing); + + VLOG(2) << TreePathFormatter(node_path) + << " ... left_spacing: " << left_spacing; } - VLOG(2) << "accrued_spaces = " << accrued_spaces; - ++column_iter; - ++properties_iter; + + ++column_config_it; + ++column_properties_it; } +} + +// Align cells by adjusting pre-token spacing for a single row. +static std::vector ComputeAlignedRowSpacings( + const VectorTree& column_configs, + const VectorTree& properties, + const AlignmentRow& row) { + VLOG(2) << __FUNCTION__ << "; row:\n" << row; + std::vector align_actions; + int accrued_spaces = 0; + + ComputeAlignedRowCellSpacings(column_configs, properties, row, &align_actions, + &accrued_spaces); + VLOG(2) << "end of " << __FUNCTION__; return align_actions; } @@ -460,12 +771,23 @@ static MutableFormatTokenRange ConvertToMutableFormatTokenRange( ConvertToMutableIterator(const_range.end(), base)); } +static const AlignmentRow* RightmostSubcolumnWithTokens( + const AlignmentRow& node) { + if (!node.Value().tokens.empty()) return &node; + for (const auto& child : reversed_view(node.Children())) { + if (child.Value().TotalWidth() > 0) { + return RightmostSubcolumnWithTokens(child); + } + } + return nullptr; +} + static FormatTokenRange EpilogRange(const TokenPartitionTree& partition, - const AlignmentRow& row) { + const AlignmentRow& last_subcol) { // Identify the unaligned epilog tokens of this 'partition', i.e. those not // spanned by 'row'. auto partition_end = partition.Value().TokensRange().end(); - auto row_end = row.back().tokens.end(); + auto row_end = last_subcol.Value().tokens.end(); return FormatTokenRange(row_end, partition_end); } @@ -473,7 +795,7 @@ static FormatTokenRange EpilogRange(const TokenPartitionTree& partition, static void CommitAlignmentDecisionToRow( const AlignmentRow& row, MutableFormatTokenRange::iterator ftoken_base, TokenPartitionTree* partition) { - if (!row.empty()) { + if (!row.Children().empty()) { const auto ftoken_range = ConvertToMutableFormatTokenRange( partition->Value().TokensRange(), ftoken_base); for (auto& ftoken : ftoken_range) { @@ -514,9 +836,11 @@ static bool AlignedRowsFitUnderColumnLimit( const AlignmentMatrix& matrix, int total_column_width, int column_limit) { auto partition_iter = rows.begin(); for (const auto& row : matrix) { - if (!row.empty()) { + const auto* rightmost_subcolumn = RightmostSubcolumnWithTokens(row); + if (rightmost_subcolumn) { // Identify the unaligned epilog text on each partition. - const FormatTokenRange epilog_range(EpilogRange(**partition_iter, row)); + const FormatTokenRange epilog_range( + EpilogRange(**partition_iter, *rightmost_subcolumn)); const int aligned_partition_width = total_column_width + EffectiveCellWidth(epilog_range); if (aligned_partition_width > column_limit) { @@ -587,16 +911,14 @@ AlignablePartitionGroup::CalculateAlignmentSpacings( cell_scanner_gen(*row)}; alignment_row_data.emplace_back(row_data); + VLOG(2) << "Row sparse columns:\n" << row_data.sparse_columns; + // Aggregate union of all column keys (syntax tree paths). column_schema.Collect(row_data.sparse_columns); } - - // Map SyntaxTreePaths to column indices. - VLOG(2) << "Mapping column indices"; - column_schema.FinalizeColumnIndices(); - const auto& column_positions = column_schema.ColumnPositions(); - const size_t num_columns = column_schema.NumUniqueColumns(); - VLOG(2) << "unique columns: " << num_columns; + VLOG(2) << "Generating column schema from collected row data"; + column_schema.Finalize(); + VLOG(2) << "Column schema:\n" << column_schema.Columns(); // Populate a matrix of cells, where cells span token ranges. // Null cells (due to optional constructs) are represented by empty ranges, @@ -606,31 +928,34 @@ AlignablePartitionGroup::CalculateAlignmentSpacings( { auto row_data_iter = alignment_row_data.cbegin(); for (auto& row : result.matrix) { - row.resize(num_columns); - FillAlignmentRow(*row_data_iter, column_positions, &row); + CopyTreeStructure( + column_schema.Columns(), &row, + [](const AggregateColumnData&) { return AlignmentCell{}; }); + + FillAlignmentRow(*row_data_iter, column_schema.SyntaxToColumnsMap(), + &row); + ComputeRowCellWidths(&row); + VLOG(2) << "Filled row:\n" << row; + ++row_data_iter; } } - // Compute compact sizes per cell. - ComputeCellWidths(&result.matrix); - // Extract other non-computed column properties. const auto column_properties = column_schema.ColumnProperties(); // Compute max widths per column. - AlignedFormattingColumnSchema column_configs( + VectorTree column_configs( ComputeColumnWidths(result.matrix, column_properties)); + VLOG(2) << "Column widths:\n" << column_configs; + { // Total width does not include initial left-indentation. // Assume indentation is the same for all partitions in each group. const int indentation = rows.front()->Value().IndentationSpaces(); - const int total_column_width = std::accumulate( - column_configs.begin(), column_configs.end(), indentation, - [](int total_width, const AlignedColumnConfiguration& c) { - return total_width + c.TotalWidth(); - }); + const int total_column_width = + indentation + column_configs.Value().TotalWidth(); VLOG(2) << "Total (aligned) column width = " << total_column_width; // if the aligned columns would exceed the column limit, then refuse to // align for now. However, this check alone does not include text that @@ -655,6 +980,7 @@ AlignablePartitionGroup::CalculateAlignmentSpacings( // Store the mutation set in a 2D structure that reflects the original token // partitions and alignment matrix representation. result.align_actions_2D.reserve(result.matrix.size()); + for (const auto& row : result.matrix) { result.align_actions_2D.push_back( ComputeAlignedRowSpacings(column_configs, column_properties, row)); diff --git a/common/formatting/align.h b/common/formatting/align.h index 76fda9e7b..e5b3dfd8a 100644 --- a/common/formatting/align.h +++ b/common/formatting/align.h @@ -27,6 +27,7 @@ #include "common/text/tree_context_visitor.h" #include "common/text/tree_utils.h" // for GetRightmostLeaf #include "common/util/logging.h" +#include "common/util/vector_tree.h" namespace verible { @@ -55,7 +56,9 @@ struct ColumnPositionEntry { AlignmentColumnProperties properties; }; -// TODO(fangism): support column groups (VectorTree) +using ColumnPositionTree = VectorTree; + +std::ostream& operator<<(std::ostream&, const ColumnPositionTree&); // ColumnSchemaScanner traverses syntax subtrees of similar types and // collects the positions that wish to register columns for alignment @@ -66,32 +69,56 @@ struct ColumnPositionEntry { // and call ReserveNewColumn() in locations that want a new columns. class ColumnSchemaScanner : public TreeContextPathVisitor { public: - ColumnSchemaScanner() = default; + ColumnSchemaScanner() + : sparse_columns_(ColumnPositionTree({{}, TokenInfo::EOFToken(), {}})) {} // Returns the collection of column position entries. - const std::vector& SparseColumns() const { - return sparse_columns_; - } + const ColumnPositionTree& SparseColumns() const { return sparse_columns_; } protected: + // Returns subpath relative to base_path + static SyntaxTreePath GetSubpath( + const SyntaxTreePath& base_path, + std::initializer_list subpositions) { + auto subpath = base_path; + subpath.insert(subpath.end(), subpositions); + return subpath; + } + // Mark the start of a new column for alignment. + // 'parent_column' is a reference to the parent column. // 'symbol' is a reference to the original source syntax subtree. // 'properties' contains alignment configuration for the column. // 'path' represents relative position within the enclosing syntax subtree, // and is used as a key for ordering and matching columns. - void ReserveNewColumn(const Symbol& symbol, - const AlignmentColumnProperties& properties, - const SyntaxTreePath& path); + ColumnPositionTree* ReserveNewColumn( + ColumnPositionTree& parent_column, const Symbol& symbol, + const AlignmentColumnProperties& properties, const SyntaxTreePath& path); + ColumnPositionTree* ReserveNewColumn( + const Symbol& symbol, const AlignmentColumnProperties& properties, + const SyntaxTreePath& path) { + return ReserveNewColumn(sparse_columns_, symbol, properties, path); + } // Reserve a column using the current path as the key. - void ReserveNewColumn(const Symbol& symbol, - const AlignmentColumnProperties& properties) { - ReserveNewColumn(symbol, properties, Path()); + ColumnPositionTree* ReserveNewColumn( + const Symbol& symbol, const AlignmentColumnProperties& properties) { + return ReserveNewColumn(sparse_columns_, symbol, properties, Path()); + } + // Reserve a subcolumn using subcolumn number appended to the parent's path + // as the key. + ColumnPositionTree* ReserveNewColumn( + ColumnPositionTree& parent_column, const Symbol& symbol, + const AlignmentColumnProperties& properties) { + auto subpath = GetSubpath(parent_column.Value().path, + {parent_column.Children().size()}); + return ReserveNewColumn(parent_column, symbol, properties, subpath); } private: // Keeps track of unique positions where new columns are desired. - std::vector sparse_columns_; + // This is a tree root and its value is not actually used. + ColumnPositionTree sparse_columns_; }; // This enum signals to the GetPartitionAlignmentSubranges() function @@ -183,7 +210,7 @@ std::vector GetPartitionAlignmentSubranges( // This is the interface used to extract alignment cells from ranges of tokens. // Note that it is not required to use a ColumnSchemaScanner. using AlignmentCellScannerFunction = - std::function(const TokenPartitionTree&)>; + std::function; // For sections of code that are deemed alignable, this enum controls // the formatter behavior. @@ -297,7 +324,7 @@ ExtractAlignmentGroupsFunction ExtractAlignmentGroupsAdapter( // Returns a sequence of column entries that will be uniquified and ordered // for alignment purposes. template -std::vector ScanPartitionForAlignmentCells( +ColumnPositionTree ScanPartitionForAlignmentCells( const TokenPartitionTree& row) { const UnwrappedLine& unwrapped_line = row.Value(); // Walk the original syntax tree that spans a subset of the tokens spanned by @@ -323,13 +350,12 @@ std::vector ScanPartitionForAlignmentCells( // and TokenPartitionTree that will be uniquified and ordered for alignment // purposes. template -std::vector -ScanPartitionForAlignmentCells_WithNonTreeTokens( +ColumnPositionTree ScanPartitionForAlignmentCells_WithNonTreeTokens( const TokenPartitionTree& row, - const std::function*)> + const std::function non_tree_column_scanner) { // re-use existing scanner - std::vector column_entries = + ColumnPositionTree column_entries = ScanPartitionForAlignmentCells(row); const UnwrappedLine& unwrapped_line = row.Value(); @@ -378,7 +404,7 @@ AlignmentCellScannerFunction AlignmentCellScannerGenerator() { // comments. template AlignmentCellScannerFunction AlignmentCellScannerGenerator( - const std::function*)> + const std::function non_tree_column_scanner) { return [non_tree_column_scanner](const TokenPartitionTree& row) { return ScanPartitionForAlignmentCells_WithNonTreeTokens( diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index 204e6530a..19450bb96 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -26,6 +26,7 @@ #include "common/text/tree_builder_test_util.h" #include "common/util/range.h" #include "common/util/spacer.h" +#include "common/util/value_saver.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -975,5 +976,351 @@ TEST_F(InferAmbiguousAlignIntentTest, DifferenceSufficientlySmall) { "threeeee four\n"); } +// Creates columns tree with the same layout as the syntax tree +template +class SyntaxTreeColumnizer : public ColumnSchemaScanner { + public: + SyntaxTreeColumnizer() = default; + + void Visit(const SyntaxTreeNode& node) override { + ColumnPositionTree* column; + if (!current_column_) + column = ReserveNewColumn(node, props); + else + column = ReserveNewColumn(*current_column_, node, props); + + ValueSaver current_column_saver(¤t_column_, column); + ColumnSchemaScanner::Visit(node); + } + void Visit(const SyntaxTreeLeaf& leaf) override { + if (!current_column_) + ReserveNewColumn(leaf, props); + else + ReserveNewColumn(*current_column_, leaf, props); + } + + protected: + ColumnPositionTree* current_column_ = nullptr; +}; + +class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture { + public: + SubcolumnsTreeAlignmentTest(absl::string_view text = + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n") + : MatrixTreeAlignmentTestFixture(text) { + // Columns tree: + // + // ┃┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┃ + // ┃┄┃┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┃┄┃ + // ┊ ┃┄┄┄┄┄┄┃┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┃┄┄┄┄┄┃ ┊ + // ┊ ┊ ┃┄┃┄┄┄┄┄┄┄┄┄┄┄┄┃┄┃ ┊ ┊ ┊ + // ┊ ┊ ┊ ┃┄┄┄┄┄┄┄┄┃┄┄┄┃ ┊ ┊ ┊ ┊ + // ┊ ┊ ┊ ┃┄┃┄┄┄┄┃┄┃ ┊ ┊ ┊ ┊ ┊ + // ┊ ┊ ┊ ┊ ┃┄┄┄┄┃ ┊ ┊ ┊ ┊ ┊ ┊ + // ┃zero ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┃ + // ┃(┃one ┃two┊ ┊ ┊ ┊ ┊ ┃three┃)┃ + // ┃(┃four ┃(┃five ┊ ┃six┃)┃ ┃seven┃)┃ + // ┃(┃eight ┃(┃(┃nine┃)┃ten┃)┃ ┃ ┃)┃ + // ┃(┃eleven┃nineteen-ninety-nine┃2k ┃)┃ + // ╵ ╵ ╵ ╵ ╵ ╵ ╵ ╵ ╵ ╵ ╵ ╵ + + syntax_tree_ = TNode(0); + + UnwrappedLine all(0, pre_format_tokens_.begin()); + all.SpanUpToToken(pre_format_tokens_.end()); + all.SetOrigin(syntax_tree_.get()); + partition_ = TokenPartitionTree{all}; + + auto token_iter = pre_format_tokens_.begin(); + while (true) { + UnwrappedLine uwline(0, token_iter); + SymbolPtr item = ParseItem(token_iter, pre_format_tokens_.end()); + if (!item.get()) { + break; + } + uwline.SpanUpToToken(token_iter); + uwline.SetOrigin(item.get()); + partition_.NewChild(uwline); + SymbolCastToNode(*syntax_tree_).AppendChild(std::move(item)); + } + } + + private: + SymbolPtr ParseList( + std::vector::iterator& it, + const std::vector::iterator& end) { + SymbolPtr list = TNode(0); + SymbolPtr item; + while ((item = ParseItem(it, end)).get() != nullptr) { + SymbolCastToNode(*list).AppendChild(std::move(item)); + } + return list; + } + + SymbolPtr ParseItem( + std::vector::iterator& it, + const std::vector::iterator& end) { + if (it == end) return SymbolPtr(nullptr); + + if (it->Text() == "(") { + SymbolPtr lp = Leaf(1, it->Text()); + ++it; + CHECK(it != end); + SymbolPtr list = ParseList(it, end); + CHECK(it != end); + CHECK_EQ(it->Text(), ")"); + SymbolPtr rp = Leaf(1, it->Text()); + ++it; + return TNode(1, std::move(lp), std::move(list), std::move(rp)); + } else if (it->Text() == ")") { + return SymbolPtr(nullptr); + } else { + SymbolPtr leaf = Leaf(0, it->Text()); + ++it; + return leaf; + } + } +}; + +static const ExtractAlignmentGroupsFunction kLeftAligningTreeAlignmentHandler = + ExtractAlignmentGroupsAdapter( + &PartitionBetweenBlankLines, &IgnoreNone, + AlignmentCellScannerGenerator>(), + AlignmentPolicy::kAlign); + +static const ExtractAlignmentGroupsFunction kRightAligningTreeAlignmentHandler = + ExtractAlignmentGroupsAdapter( + &PartitionBetweenBlankLines, &IgnoreNone, + AlignmentCellScannerGenerator>(), + AlignmentPolicy::kAlign); + +static const ExtractAlignmentGroupsFunction kFlushLeftTreeAlignmentHandler = + ExtractAlignmentGroupsAdapter( + &PartitionBetweenBlankLines, &IgnoreNone, + AlignmentCellScannerGenerator>(), + AlignmentPolicy::kFlushLeft); + +static const ExtractAlignmentGroupsFunction kPreserveTreeAlignmentHandler = + ExtractAlignmentGroupsAdapter( + &PartitionBetweenBlankLines, &IgnoreNone, + AlignmentCellScannerGenerator>(), + AlignmentPolicy::kPreserve); + +TEST_F(SubcolumnsTreeAlignmentTest, ZeroInterTokenPadding) { + TabularAlignTokens(&partition_, kLeftAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "(one two three)\n" + "(four (five six) seven)\n" + "(eight ((nine)ten) )\n" + "(elevennineteen-ninety-nine2k )\n"); +} + +TEST_F(SubcolumnsTreeAlignmentTest, AlignmentPolicyFlushLeft) { + TabularAlignTokens(&partition_, kFlushLeftTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "(onetwothree)\n" + "(four(fivesix)seven)\n" + "(eight((nine)ten))\n" + "(elevennineteen-ninety-nine2k)\n"); +} + +TEST_F(SubcolumnsTreeAlignmentTest, AlignmentPolicyPreserve) { + ConnectPreFormatTokensPreservedSpaceStarts(sample_.data(), + &pre_format_tokens_); + + TabularAlignTokens(&partition_, kPreserveTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n"); +} + +TEST_F(SubcolumnsTreeAlignmentTest, OneInterTokenPadding) { + for (auto& ftoken : pre_format_tokens_) { + ftoken.before.spaces_required = 1; + } + + TabularAlignTokens(&partition_, kLeftAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n"); +} + +TEST_F(SubcolumnsTreeAlignmentTest, OneInterTokenPaddingExceptFront) { + for (auto& ftoken : pre_format_tokens_) { + ftoken.before.spaces_required = 1; + } + // Find first token of each line and require 0 spaces before them. + for (auto& line : partition_.Children()) { + const auto tokens = line.Value().TokensRange(); + if (!tokens.empty()) { + const PreFormatToken& front = tokens.front(); + auto mutable_token = + std::find_if(pre_format_tokens_.begin(), pre_format_tokens_.end(), + [&](const PreFormatToken& ftoken) { + return BoundsEqual(ftoken.Text(), front.Text()); + }); + if (mutable_token != pre_format_tokens_.end()) { + mutable_token->before.spaces_required = 0; + } + } + } + + TabularAlignTokens(&partition_, kLeftAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n"); +} + +TEST_F(SubcolumnsTreeAlignmentTest, RightFlushed) { + for (auto& ftoken : pre_format_tokens_) { + ftoken.before.spaces_required = 1; + } + + TabularAlignTokens(&partition_, kRightAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + " zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n"); +} + +TEST_F(SubcolumnsTreeAlignmentTest, + RightFlushedOneInterTokenPaddingWithIndent) { + for (auto& ftoken : pre_format_tokens_) { + ftoken.before.spaces_required = 1; + } + for (auto& line : partition_.Children()) { + line.Value().SetIndentationSpaces(2); + } + + TabularAlignTokens(&partition_, kRightAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + " zero\n" + " ( one two three )\n" + " ( four ( five six ) seven )\n" + " ( eight ( ( nine ) ten ) )\n" + " ( eleven nineteen-ninety-nine 2k )\n"); +} + +class MultiSubcolumnsTreeAlignmentTest : public SubcolumnsTreeAlignmentTest { + public: + MultiSubcolumnsTreeAlignmentTest(absl::string_view text = + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n") + : SubcolumnsTreeAlignmentTest(text) {} + + std::string Render() const { + std::ostringstream stream; + int position = 0; + const absl::string_view text(sample_); + for (const auto& child : partition_.Children()) { + // emulate preserving vertical spacing + const auto tokens_range = child.Value().TokensRange(); + const auto front_offset = tokens_range.front().token->left(text); + const absl::string_view spaces = + text.substr(position, front_offset - position); + const auto newlines = + std::max(std::count(spaces.begin(), spaces.end(), '\n') - 1, 0); + stream << Spacer(newlines, '\n'); + stream << FormattedExcerpt(child.Value()) << std::endl; + position = tokens_range.back().token->right(text); + } + return stream.str(); + } +}; + +TEST_F(MultiSubcolumnsTreeAlignmentTest, BlankLineSeparatedGroups) { + for (auto& ftoken : pre_format_tokens_) { + ftoken.before.spaces_required = 1; + } + + TabularAlignTokens(&partition_, kLeftAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n"); +} + +class InferSubcolumnsTreeAlignmentTest : public SubcolumnsTreeAlignmentTest { + public: + InferSubcolumnsTreeAlignmentTest( + absl::string_view text = + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n") + : SubcolumnsTreeAlignmentTest(text) { + // Need to know original spacing to be able to infer user-intent. + ConnectPreFormatTokensPreservedSpaceStarts(sample_.data(), + &pre_format_tokens_); + + // Require 1 space between tokens. + for (auto& ftoken : pre_format_tokens_) { + ftoken.before.spaces_required = 1; + // Default to append, so we can see the effect of falling-back to + // preserve-spacing behavior. + ftoken.before.break_decision = SpacingOptions::MustAppend; + } + } +}; + +static const ExtractAlignmentGroupsFunction kInferTreeAlignmentHandler = + ExtractAlignmentGroupsAdapter( + &PartitionBetweenBlankLines, &IgnoreNone, + AlignmentCellScannerGenerator>(), + AlignmentPolicy::kInferUserIntent); + +TEST_F(InferSubcolumnsTreeAlignmentTest, InferUserIntent) { + TabularAlignTokens(&partition_, kInferTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "zero\n" + "( one two three )\n" + "( four ( five six ) seven )\n" + "( eight ( ( nine ) ten ) )\n" + "( eleven nineteen-ninety-nine 2k )\n"); +} + } // namespace } // namespace verible diff --git a/common/util/vector_tree.h b/common/util/vector_tree.h index 6fe2dbf30..669755820 100644 --- a/common/util/vector_tree.h +++ b/common/util/vector_tree.h @@ -238,12 +238,11 @@ class VectorTree : private _VectorTreeImpl { typedef _VectorTreeImpl impl_type; - // Private default constructor. - VectorTree() = default; - public: typedef T value_type; + VectorTree() = default; + // Deep copy-constructor. VectorTree(const this_type& other) : node_value_(other.node_value_), diff --git a/verilog/formatting/align.cc b/verilog/formatting/align.cc index 64bdf6581..ea93cb2f3 100644 --- a/verilog/formatting/align.cc +++ b/verilog/formatting/align.cc @@ -1132,7 +1132,7 @@ using AlignmentHandlerMapType = static void non_tree_column_scanner( verible::TokenRange token_range, - std::vector* column_entries) { + verible::ColumnPositionTree* column_entries) { static const size_t kLargestPathIndex = std::numeric_limits::max(); for (auto token : token_range) { @@ -1141,15 +1141,15 @@ static void non_tree_column_scanner( SyntaxTreePath path{kLargestPathIndex - 1}; AlignmentColumnProperties prop; prop.contains_delimiter = true; - verible::ColumnPositionEntry test{path, token, prop}; - column_entries->push_back(test); + const verible::ColumnPositionTree column({path, token, prop}); + column_entries->NewChild(column); break; } case TK_COMMENT_BLOCK: case TK_EOL_COMMENT: { SyntaxTreePath path{kLargestPathIndex}; - verible::ColumnPositionEntry test{path, token, FlushLeft}; - column_entries->push_back(test); + const verible::ColumnPositionTree column({path, token, FlushLeft}); + column_entries->NewChild(column); break; } default: From 7c5db6b25f31a1205a268909a3706238f06c2b02 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Sat, 10 Jul 2021 00:49:20 +0200 Subject: [PATCH 03/23] Align dimensions and ranges in class properties and dist items --- verilog/formatting/align.cc | 52 +++++++++++++++-- verilog/formatting/formatter_test.cc | 87 ++++++++++++++++++++++++++-- 2 files changed, 129 insertions(+), 10 deletions(-) diff --git a/verilog/formatting/align.cc b/verilog/formatting/align.cc index ea93cb2f3..3de79f9dd 100644 --- a/verilog/formatting/align.cc +++ b/verilog/formatting/align.cc @@ -745,6 +745,31 @@ class ClassPropertyColumnSchemaScanner : public ColumnSchemaScanner { ReserveNewColumn(node, FlushLeft); break; } + case NodeEnum::kDimensionScalar: { + CHECK_EQ(node.children().size(), 3); + auto* column = ABSL_DIE_IF_NULL(ReserveNewColumn(node, FlushLeft)); + + ReserveNewColumn(*column, *node[0], FlushLeft); // '[' + ReserveNewColumn(*column, *node[1], FlushRight); // value + ReserveNewColumn(*column, *node[2], FlushLeft); // ']' + return; + } + case NodeEnum::kDimensionRange: { + CHECK_EQ(node.children().size(), 5); + auto* column = ABSL_DIE_IF_NULL(ReserveNewColumn(node, FlushLeft)); + + SyntaxTreePath np; + ReserveNewColumn(*column, *node[0], FlushLeft); // '[' + + auto* value_subcolumn = + ABSL_DIE_IF_NULL(ReserveNewColumn(*column, *node[1], FlushRight)); + ReserveNewColumn(*value_subcolumn, *node[1], FlushRight); // LHS value + ReserveNewColumn(*value_subcolumn, *node[2], FlushLeft); // ':' + ReserveNewColumn(*value_subcolumn, *node[3], FlushLeft); // RHS value + + ReserveNewColumn(*column, *node[4], FlushLeft); // ']' + return; + } default: break; } @@ -1077,11 +1102,27 @@ class DistItemColumnSchemaScanner : public ColumnSchemaScanner { switch (tag) { case NodeEnum::kDistributionItem: // Start first column right away. - ReserveNewColumn(node, FlushLeft); + item_column_ = ReserveNewColumn(node, FlushLeft); break; - - // TODO(fangism): the left-hand-side may contain [x:y] ranges that could - // be further aligned. + case NodeEnum::kValueRange: { + if (!Context().DirectParentIs(NodeEnum::kDistributionItem)) { + break; + } + CHECK_EQ(node.children().size(), 5); + CHECK_NOTNULL(item_column_); + ReserveNewColumn(*item_column_, *node[0], FlushLeft, + GetSubpath(Path(), {0})); // '[' + ReserveNewColumn(*item_column_, *node[1], FlushRight, + GetSubpath(Path(), {1})); // LHS value + ReserveNewColumn(*item_column_, *node[2], FlushLeft, + GetSubpath(Path(), {2})); // ':' + ReserveNewColumn(*item_column_, *node[3], FlushLeft, + GetSubpath(Path(), {3})); // RHS value + ReserveNewColumn(*item_column_, *node[4], FlushLeft, + GetSubpath(Path(), {4})); // ']' + item_column_ = nullptr; + return; + } default: break; } @@ -1100,6 +1141,9 @@ class DistItemColumnSchemaScanner : public ColumnSchemaScanner { break; } } + + private: + verible::ColumnPositionTree* item_column_ = nullptr; }; static std::function< diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index e4e2f9933..22be1c1d7 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -3755,9 +3755,38 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { "int foo_bar ;\n" "endclass : c\n", "class c;\n" - " int foo[$];\n" + " int foo [$];\n" " int foo_bar;\n" "endclass : c\n"}, + {// subcolumns + "class cc;\n" + "rand bit [A-1:0] foo;\n" + "rand bit [A-1:0][2] bar;\n" + "int foobar[X+1:Y];\n" + "int baz[42];\n" + "rand bit qux[Z];\n" + "rand bit [1:0] quux[3:0];\n" + "rand bit [A:BB][42] quuz[7];\n" + "endclass\n", + "class cc;\n" + " rand bit [A-1:0 ] foo;\n" + " rand bit [A-1:0 ][ 2] bar;\n" + " int foobar[X+1:Y];\n" + " int baz [ 42];\n" + " rand bit qux [ Z];\n" + " rand bit [ 1:0 ] quux [ 3:0];\n" + " rand bit [ A:BB][42] quuz [ 7];\n" + "endclass\n"}, + {"class cc;\n" + "int qux[2];\n" + "int quux[SIZE-1+SHIFT:SHIFT];\n" + "int quuz[SOME_CONSTANT];\n" + "endclass\n", + "class cc;\n" + " int qux [ 2];\n" + " int quux[SIZE-1+SHIFT:SHIFT];\n" + " int quuz[ SOME_CONSTANT];\n" + "endclass\n"}, {// aligns over comments (ignored) "class c;\n" "// foo is...\n" @@ -3796,14 +3825,13 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { " // llama is...\n" " logic llama;\n" "endclass : c\n"}, - {// array dimensions do not have their own columns - "class c;\n" + {"class c;\n" "rand logic l;\n" "int [1:0] foo;\n" "endclass : c\n", "class c;\n" - " rand logic l;\n" - " int [1:0] foo;\n" + " rand logic l;\n" + " int [1:0] foo;\n" "endclass : c\n"}, {// non-data-declarations break up groups "class c;\n" @@ -3947,7 +3975,54 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { " }\n" "endclass\n", }, - + // distributions: colon alignment + {"class c;\n" + "constraint co {\n" + "d dist {\n" + "[1:2]:/2,\n" + "[11:33]:/22,\n" + "[111:444]:/8,\n" + "[1:42]:/10,\n" + "[11:12]:/3\n" + "};\n" + "}\n" + "endclass\n", + "class c;\n" + " constraint co {\n" + " d dist {\n" + " [ 1 : 2 ] :/ 2,\n" + " [ 11 : 33 ] :/ 22,\n" + " [111 : 444] :/ 8,\n" + " [ 1 : 42 ] :/ 10,\n" + " [ 11 : 12 ] :/ 3\n" + " };\n" + " }\n" + "endclass\n"}, + // distributions: subcolumns + {"class foo;\n" + "constraint bar {\n" + "baz dist {\n" + "[1:2]:/2,\n" + "QUX[3:0]:/10,\n" + "[11:33]:/22,\n" + "ID_LONGER_THAN_RANGES:/3,\n" + "[111:QUUZ[Z]]:/8,\n" + "[X[4:0]:Y[8:Z-2]]:/8\n" + "};\n" + "}\n" + "endclass\n", + "class foo;\n" + " constraint bar {\n" + " baz dist {\n" + " [ 1 : 2 ] :/ 2,\n" + " QUX[3:0] :/ 10,\n" + " [ 11 : 33 ] :/ 22,\n" + " ID_LONGER_THAN_RANGES :/ 3,\n" + " [ 111 : QUUZ[Z] ] :/ 8,\n" + " [X[4:0] : Y[8:Z-2]] :/ 8\n" + " };\n" + " }\n" + "endclass\n"}, // class with empty parameter list {"class foo #(); endclass", "class foo #();\n" From 5f03ec6465f01c6f04b71279942707adcc6f320b Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Wed, 28 Jul 2021 22:52:57 +0200 Subject: [PATCH 04/23] Improve logger readability --- common/formatting/align.cc | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index bfa7faea5..90ee68e42 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -412,9 +412,6 @@ class ColumnSchemaAggregator { template static std::pair GetColumnDataCellLabel( const VectorTree& node) { - const absl::string_view arrow = - node.Value().properties.flush_left ? "<" : ">"; - std::ostringstream label; const auto& path = node.Value().path; auto begin = path.begin(); @@ -428,12 +425,12 @@ static std::pair GetColumnDataCellLabel( ++parent_begin; } } - label << " " << arrow << "\t"; + label << " \t "; if (begin != path.begin() && begin != path.end()) label << "."; label << SequenceFormatter(iterator_range(begin, path.end()), "."); - label << "\t" << arrow << " "; + label << " \t "; - return {label.str(), '-'}; + return {label.str(), node.Value().properties.flush_left ? '<' : '>'}; } std::ostream& operator<<(std::ostream& stream, @@ -619,7 +616,7 @@ static AlignedFormattingColumnSchema ComputeColumnWidths( return width + node.Value().TotalWidth(); }); cell.Value().left_border = - std::max(cell.Value().left_border, + std::min(cell.Value().left_border, cell.Children().front().Value().left_border); cell.Value().width = std::max(cell.Value().width, children_width - cell.Value().left_border); From 9aeac4430a2a9bc2f61ef62c2fe96b6c88631461 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Thu, 29 Jul 2021 13:25:06 +0200 Subject: [PATCH 05/23] Use single name to refer to the same thing --- common/formatting/align.cc | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 90ee68e42..6a48b154a 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -608,18 +608,19 @@ static AlignedFormattingColumnSchema ComputeColumnWidths( } } - for (auto& cell : VectorTreePostOrderTraversal(column_configs)) { - if (!cell.is_leaf()) { + for (auto& column_iter : VectorTreePostOrderTraversal(column_configs)) { + if (!column_iter.is_leaf()) { int children_width = std::accumulate( - cell.Children().begin(), cell.Children().end(), 0, + column_iter.Children().begin(), column_iter.Children().end(), 0, [](int width, const AlignedFormattingColumnSchema& node) { return width + node.Value().TotalWidth(); }); - cell.Value().left_border = - std::min(cell.Value().left_border, - cell.Children().front().Value().left_border); - cell.Value().width = std::max(cell.Value().width, - children_width - cell.Value().left_border); + column_iter.Value().left_border = + std::max(column_iter.Value().left_border, + column_iter.Children().front().Value().left_border); + column_iter.Value().width = + std::max(column_iter.Value().width, + children_width - column_iter.Value().left_border); } } From 9661062a65bcbe2d974832b070d77283f6700dcd Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Mon, 23 Aug 2021 15:26:31 +0200 Subject: [PATCH 06/23] Pass modified arguments as pointers --- common/formatting/align.cc | 15 +++++++------- common/formatting/align.h | 13 ++++++------ common/formatting/align_test.cc | 35 +++++++++++++++++---------------- verilog/formatting/align.cc | 28 +++++++++++++------------- 4 files changed, 47 insertions(+), 44 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 6a48b154a..731a9a6f6 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -271,33 +271,34 @@ struct AlignedColumnConfiguration { }; ColumnPositionTree* ColumnSchemaScanner::ReserveNewColumn( - ColumnPositionTree& parent_column, const Symbol& symbol, + ColumnPositionTree* parent_column, const Symbol& symbol, const AlignmentColumnProperties& properties, const SyntaxTreePath& path) { + CHECK_NOTNULL(parent_column); // The path helps establish a total ordering among all desired alignment // points, given that they may come from optional or repeated language // constructs. const SyntaxTreeLeaf* leaf = GetLeftmostLeaf(symbol); // It is possible for a node to be empty, in which case, ignore. if (leaf == nullptr) return nullptr; - if (parent_column.Parent() != nullptr && parent_column.Children().empty()) { + if (parent_column->Parent() != nullptr && parent_column->Children().empty()) { // Starting token of a column and its first subcolumn must be the same. // (subcolumns overlap their parent column). - CHECK_EQ(parent_column.Value().starting_token, leaf->get()); + CHECK_EQ(parent_column->Value().starting_token, leaf->get()); } // It's possible the previous cell's path was intentionally altered // to effectively fuse it with the cell that is about to be added. // When this occurs, take the (previous) leftmost token, and suppress // adding a new column. - if (parent_column.Children().empty() || - parent_column.Children().back().Value().path != path) { - const auto* column = parent_column.NewChild( + if (parent_column->Children().empty() || + parent_column->Children().back().Value().path != path) { + const auto* column = parent_column->NewChild( ColumnPositionEntry{path, leaf->get(), properties}); ColumnsTreePath column_path; column->Path(column_path); VLOG(2) << "reserving new column for " << TreePathFormatter(path) << " at " << TreePathFormatter(column_path); } - return &parent_column.Children().back(); + return &parent_column->Children().back(); } struct AggregateColumnData { diff --git a/common/formatting/align.h b/common/formatting/align.h index e5b3dfd8a..119d75282 100644 --- a/common/formatting/align.h +++ b/common/formatting/align.h @@ -92,26 +92,27 @@ class ColumnSchemaScanner : public TreeContextPathVisitor { // 'path' represents relative position within the enclosing syntax subtree, // and is used as a key for ordering and matching columns. ColumnPositionTree* ReserveNewColumn( - ColumnPositionTree& parent_column, const Symbol& symbol, + ColumnPositionTree* parent_column, const Symbol& symbol, const AlignmentColumnProperties& properties, const SyntaxTreePath& path); ColumnPositionTree* ReserveNewColumn( const Symbol& symbol, const AlignmentColumnProperties& properties, const SyntaxTreePath& path) { - return ReserveNewColumn(sparse_columns_, symbol, properties, path); + return ReserveNewColumn(&sparse_columns_, symbol, properties, path); } // Reserve a column using the current path as the key. ColumnPositionTree* ReserveNewColumn( const Symbol& symbol, const AlignmentColumnProperties& properties) { - return ReserveNewColumn(sparse_columns_, symbol, properties, Path()); + return ReserveNewColumn(&sparse_columns_, symbol, properties, Path()); } // Reserve a subcolumn using subcolumn number appended to the parent's path // as the key. ColumnPositionTree* ReserveNewColumn( - ColumnPositionTree& parent_column, const Symbol& symbol, + ColumnPositionTree* parent_column, const Symbol& symbol, const AlignmentColumnProperties& properties) { - auto subpath = GetSubpath(parent_column.Value().path, - {parent_column.Children().size()}); + CHECK_NOTNULL(parent_column); + auto subpath = GetSubpath(parent_column->Value().path, + {parent_column->Children().size()}); return ReserveNewColumn(parent_column, symbol, properties, subpath); } diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index 19450bb96..0e441e6d8 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -987,7 +987,7 @@ class SyntaxTreeColumnizer : public ColumnSchemaScanner { if (!current_column_) column = ReserveNewColumn(node, props); else - column = ReserveNewColumn(*current_column_, node, props); + column = ReserveNewColumn(current_column_, node, props); ValueSaver current_column_saver(¤t_column_, column); ColumnSchemaScanner::Visit(node); @@ -996,7 +996,7 @@ class SyntaxTreeColumnizer : public ColumnSchemaScanner { if (!current_column_) ReserveNewColumn(leaf, props); else - ReserveNewColumn(*current_column_, leaf, props); + ReserveNewColumn(current_column_, leaf, props); } protected: @@ -1038,7 +1038,7 @@ class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture { auto token_iter = pre_format_tokens_.begin(); while (true) { UnwrappedLine uwline(0, token_iter); - SymbolPtr item = ParseItem(token_iter, pre_format_tokens_.end()); + SymbolPtr item = ParseItem(&token_iter, pre_format_tokens_.end()); if (!item.get()) { break; } @@ -1051,7 +1051,7 @@ class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture { private: SymbolPtr ParseList( - std::vector::iterator& it, + std::vector::iterator* it, const std::vector::iterator& end) { SymbolPtr list = TNode(0); SymbolPtr item; @@ -1062,25 +1062,26 @@ class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture { } SymbolPtr ParseItem( - std::vector::iterator& it, + std::vector::iterator* it, const std::vector::iterator& end) { - if (it == end) return SymbolPtr(nullptr); + CHECK_NOTNULL(it); + if (*it == end) return SymbolPtr(nullptr); - if (it->Text() == "(") { - SymbolPtr lp = Leaf(1, it->Text()); - ++it; - CHECK(it != end); + if ((*it)->Text() == "(") { + SymbolPtr lp = Leaf(1, (*it)->Text()); + ++*it; + CHECK(*it != end); SymbolPtr list = ParseList(it, end); - CHECK(it != end); - CHECK_EQ(it->Text(), ")"); - SymbolPtr rp = Leaf(1, it->Text()); - ++it; + CHECK(*it != end); + CHECK_EQ((*it)->Text(), ")"); + SymbolPtr rp = Leaf(1, (*it)->Text()); + ++*it; return TNode(1, std::move(lp), std::move(list), std::move(rp)); - } else if (it->Text() == ")") { + } else if ((*it)->Text() == ")") { return SymbolPtr(nullptr); } else { - SymbolPtr leaf = Leaf(0, it->Text()); - ++it; + SymbolPtr leaf = Leaf(0, (*it)->Text()); + ++*it; return leaf; } } diff --git a/verilog/formatting/align.cc b/verilog/formatting/align.cc index 3de79f9dd..c6edac1b1 100644 --- a/verilog/formatting/align.cc +++ b/verilog/formatting/align.cc @@ -749,9 +749,9 @@ class ClassPropertyColumnSchemaScanner : public ColumnSchemaScanner { CHECK_EQ(node.children().size(), 3); auto* column = ABSL_DIE_IF_NULL(ReserveNewColumn(node, FlushLeft)); - ReserveNewColumn(*column, *node[0], FlushLeft); // '[' - ReserveNewColumn(*column, *node[1], FlushRight); // value - ReserveNewColumn(*column, *node[2], FlushLeft); // ']' + ReserveNewColumn(column, *node[0], FlushLeft); // '[' + ReserveNewColumn(column, *node[1], FlushRight); // value + ReserveNewColumn(column, *node[2], FlushLeft); // ']' return; } case NodeEnum::kDimensionRange: { @@ -759,15 +759,15 @@ class ClassPropertyColumnSchemaScanner : public ColumnSchemaScanner { auto* column = ABSL_DIE_IF_NULL(ReserveNewColumn(node, FlushLeft)); SyntaxTreePath np; - ReserveNewColumn(*column, *node[0], FlushLeft); // '[' + ReserveNewColumn(column, *node[0], FlushLeft); // '[' auto* value_subcolumn = - ABSL_DIE_IF_NULL(ReserveNewColumn(*column, *node[1], FlushRight)); - ReserveNewColumn(*value_subcolumn, *node[1], FlushRight); // LHS value - ReserveNewColumn(*value_subcolumn, *node[2], FlushLeft); // ':' - ReserveNewColumn(*value_subcolumn, *node[3], FlushLeft); // RHS value + ABSL_DIE_IF_NULL(ReserveNewColumn(column, *node[1], FlushRight)); + ReserveNewColumn(value_subcolumn, *node[1], FlushRight); // LHS value + ReserveNewColumn(value_subcolumn, *node[2], FlushLeft); // ':' + ReserveNewColumn(value_subcolumn, *node[3], FlushLeft); // RHS value - ReserveNewColumn(*column, *node[4], FlushLeft); // ']' + ReserveNewColumn(column, *node[4], FlushLeft); // ']' return; } default: @@ -1110,15 +1110,15 @@ class DistItemColumnSchemaScanner : public ColumnSchemaScanner { } CHECK_EQ(node.children().size(), 5); CHECK_NOTNULL(item_column_); - ReserveNewColumn(*item_column_, *node[0], FlushLeft, + ReserveNewColumn(item_column_, *node[0], FlushLeft, GetSubpath(Path(), {0})); // '[' - ReserveNewColumn(*item_column_, *node[1], FlushRight, + ReserveNewColumn(item_column_, *node[1], FlushRight, GetSubpath(Path(), {1})); // LHS value - ReserveNewColumn(*item_column_, *node[2], FlushLeft, + ReserveNewColumn(item_column_, *node[2], FlushLeft, GetSubpath(Path(), {2})); // ':' - ReserveNewColumn(*item_column_, *node[3], FlushLeft, + ReserveNewColumn(item_column_, *node[3], FlushLeft, GetSubpath(Path(), {3})); // RHS value - ReserveNewColumn(*item_column_, *node[4], FlushLeft, + ReserveNewColumn(item_column_, *node[4], FlushLeft, GetSubpath(Path(), {4})); // ']' item_column_ = nullptr; return; From c38eab11e46a798a59a8ebc662581a652b64544f Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Mon, 23 Aug 2021 15:15:57 +0200 Subject: [PATCH 07/23] Do not use protected scope when it is not needed --- common/formatting/align_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index 0e441e6d8..0fc1364c0 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -999,7 +999,7 @@ class SyntaxTreeColumnizer : public ColumnSchemaScanner { ReserveNewColumn(current_column_, leaf, props); } - protected: + private: ColumnPositionTree* current_column_ = nullptr; }; From 73c8be04b4f99ce9da730bf521c77f84c386ba77 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Mon, 23 Aug 2021 16:10:41 +0200 Subject: [PATCH 08/23] Put VectorTreeIteratorBase in `internal` namespace --- common/util/vector_tree_iterators.h | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/common/util/vector_tree_iterators.h b/common/util/vector_tree_iterators.h index 1d9e53691..5ce2b1038 100644 --- a/common/util/vector_tree_iterators.h +++ b/common/util/vector_tree_iterators.h @@ -23,12 +23,14 @@ namespace verible { +namespace internal { + // Class implementing common VectorTree*Iterator members using CRTP polymorphic // chaining. Derived class must implement following methods: // - `static VectorTreeType* _NextNode(VectorTreeType* node)` - returns pointer // to a next node template -class _VectorTreeIteratorBase { +class VectorTreeIteratorBase { public: using iterator_category = std::forward_iterator_tag; using difference_type = std::ptrdiff_t; @@ -36,8 +38,8 @@ class _VectorTreeIteratorBase { using pointer = VectorTreeType*; using reference = VectorTreeType&; - _VectorTreeIteratorBase() : node_(nullptr) {} - _VectorTreeIteratorBase(pointer node) : node_(node) {} + VectorTreeIteratorBase() : node_(nullptr) {} + VectorTreeIteratorBase(pointer node) : node_(node) {} reference operator*() const { CHECK_NOTNULL(node_); @@ -71,12 +73,14 @@ class _VectorTreeIteratorBase { pointer node_; }; +} // namespace internal + template class VectorTreeLeavesIterator - : public _VectorTreeIteratorBase, - VectorTreeType> { + : public internal::VectorTreeIteratorBase< + VectorTreeLeavesIterator, VectorTreeType> { using this_type = VectorTreeLeavesIterator; - using base_type = _VectorTreeIteratorBase; + using base_type = internal::VectorTreeIteratorBase; public: VectorTreeLeavesIterator() : base_type() {} @@ -103,10 +107,10 @@ VectorTreeLeavesTraversal(VectorTreeType& tree) { template class VectorTreePreOrderIterator - : public _VectorTreeIteratorBase, - VectorTreeType> { + : public internal::VectorTreeIteratorBase< + VectorTreePreOrderIterator, VectorTreeType> { using this_type = VectorTreePreOrderIterator; - using base_type = _VectorTreeIteratorBase; + using base_type = internal::VectorTreeIteratorBase; public: VectorTreePreOrderIterator() : base_type() {} @@ -140,10 +144,10 @@ VectorTreePreOrderTraversal(VectorTreeType& tree) { template class VectorTreePostOrderIterator - : public _VectorTreeIteratorBase< + : public internal::VectorTreeIteratorBase< VectorTreePostOrderIterator, VectorTreeType> { using this_type = VectorTreePostOrderIterator; - using base_type = _VectorTreeIteratorBase; + using base_type = internal::VectorTreeIteratorBase; public: VectorTreePostOrderIterator() : base_type() {} From 28c857dcf8fa663c4475fd5712e9fa995cd7f79e Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Mon, 23 Aug 2021 17:42:56 +0200 Subject: [PATCH 09/23] Add type alias for get_cell_label --- common/formatting/align.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 731a9a6f6..cc52fec42 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -130,11 +130,14 @@ std::ostream& operator<<(std::ostream& stream, const AlignmentCell& cell) { return stream; } +template +using CellLabelGetterFunc = + std::function(const VectorTree&)>; + template static std::size_t CreateTextNodes( const VectorTree& src_node, VectorTree* dst_node, - const std::function( - const VectorTree&)>& get_cell_label) { + const CellLabelGetterFunc& get_cell_label) { static constexpr std::size_t kMinCellWidth = 2; std::size_t depth = 0; @@ -158,8 +161,7 @@ static std::size_t CreateTextNodes( template static void ColumnsTreeFormatter( std::ostream& stream, const VectorTree& root, - const std::function( - const VectorTree&)>& get_cell_label) { + const CellLabelGetterFunc& get_cell_label) { if (root.Children().empty()) return; static constexpr absl::string_view kCellSeparator = "|"; From 0ace6ab213bd907e52dedc4d58c402821f9bb287 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Tue, 24 Aug 2021 14:07:21 +0200 Subject: [PATCH 10/23] Use switch statement --- common/formatting/align.cc | 41 ++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index cc52fec42..a8fbd1bb8 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -235,23 +235,30 @@ static void ColumnsTreeFormatter( const auto width = cell.width - kCellSeparator.size(); - if (parts.size() == 1) { - const std::string pad(width - parts[0].size(), cell.filler); - absl::StrAppend(&lines[level], kCellSeparator, parts[0], pad); - } else if (parts.size() == 2) { - const std::string pad(width - parts[0].size() - parts[1].size(), - cell.filler); - absl::StrAppend(&lines[level], kCellSeparator, parts[0], pad, - parts.back()); - } else if (parts.size() == 3) { - std::size_t pos = - std::clamp((width - parts[1].size()) / 2, parts[0].size() + 1, - width - parts[2].size() - parts[1].size() - 1); - const std::string left_pad(pos - parts[0].size(), cell.filler); - const std::string right_pad( - width - parts[2].size() - (pos + parts[1].size()), cell.filler); - absl::StrAppend(&lines[level], kCellSeparator, parts[0], left_pad, - parts[1], right_pad, parts[2]); + switch (parts.size()) { + case 1: { + const std::string pad(width - parts[0].size(), cell.filler); + absl::StrAppend(&lines[level], kCellSeparator, parts[0], pad); + break; + } + case 2: { + const std::string pad(width - parts[0].size() - parts[1].size(), + cell.filler); + absl::StrAppend(&lines[level], kCellSeparator, parts[0], pad, + parts.back()); + break; + } + case 3: { + std::size_t pos = + std::clamp((width - parts[1].size()) / 2, parts[0].size() + 1, + width - parts[2].size() - parts[1].size() - 1); + const std::string left_pad(pos - parts[0].size(), cell.filler); + const std::string right_pad( + width - parts[2].size() - (pos + parts[1].size()), cell.filler); + absl::StrAppend(&lines[level], kCellSeparator, parts[0], left_pad, + parts[1], right_pad, parts[2]); + break; + } } } for (const auto& line : lines) { From 67b9d13c5c1291f11224d6bb6b774edf0172d632 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Tue, 24 Aug 2021 17:00:44 +0200 Subject: [PATCH 11/23] Document return value --- common/formatting/align.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common/formatting/align.h b/common/formatting/align.h index 119d75282..1e772fe6e 100644 --- a/common/formatting/align.h +++ b/common/formatting/align.h @@ -86,11 +86,12 @@ class ColumnSchemaScanner : public TreeContextPathVisitor { } // Mark the start of a new column for alignment. - // 'parent_column' is a reference to the parent column. + // 'parent_column' is a pointer to the parent column. // 'symbol' is a reference to the original source syntax subtree. // 'properties' contains alignment configuration for the column. // 'path' represents relative position within the enclosing syntax subtree, // and is used as a key for ordering and matching columns. + // Returns pointer to a created column or nullptr if column was not created. ColumnPositionTree* ReserveNewColumn( ColumnPositionTree* parent_column, const Symbol& symbol, const AlignmentColumnProperties& properties, const SyntaxTreePath& path); From f8122f4baab13692432a127a43814b9711c8271b Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Wed, 25 Aug 2021 13:35:08 +0200 Subject: [PATCH 12/23] Add documentation comments --- common/formatting/align.cc | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index a8fbd1bb8..c0f0c31fb 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -130,10 +130,18 @@ std::ostream& operator<<(std::ostream& stream, const AlignmentCell& cell) { return stream; } +// Type of functions used to generate textual node representations that are +// suitable for use in rectangular cell. +// The function is called with a tree node as its only argument. It should +// return a string containing the cell's text and a single character used as +// a filler for cell's empty space. template using CellLabelGetterFunc = std::function(const VectorTree&)>; +// Recursively creates a tree with cells textual data. Its main purpose is to +// split multi-line cell labels and calculate how many lines have to be printed. +// This is a helper function used in ColumsTreeFormatter. template static std::size_t CreateTextNodes( const VectorTree& src_node, VectorTree* dst_node, @@ -158,6 +166,15 @@ static std::size_t CreateTextNodes( return depth + subtree_depth; } +// Prints visualization of columns tree 'root' to a 'stream'. The 'root' node +// itself is not visualized. The 'get_cell_label' callback is used to get the +// cell label printed for each node. +// +// The label's text can contain multiple lines. Each line can contain up to 3 +// fields separated by tab character ('\t'). The first field is aligned to the +// left. The second field is either aligned to the right (when there are +// 2 fields) or centered (when there are 3 fields). The third field is aligned +// to the right. Empty space is filled with label's filler character. template static void ColumnsTreeFormatter( std::ostream& stream, const VectorTree& root, @@ -333,6 +350,9 @@ struct AggregateColumnData { } }; +// Creates 'dst_tree' tree with the same structure as 'src_tree'. Value of each +// created node is obtained by calling 'converter' function with corresponding +// node from 'src_tree'. template static void CopyTreeStructure( const VectorTree& src_tree, @@ -419,6 +439,9 @@ class ColumnSchemaAggregator { std::map syntax_to_columns_map_; }; +// CellLabelGetterFunc which creates a label with column's path relative to +// its parent column and either '<' or '>' filler characters indicating whether +// the column flushes to the left or the right. template static std::pair GetColumnDataCellLabel( const VectorTree& node) { @@ -532,6 +555,8 @@ static void FillAlignmentRow( } } +// Recursively calculates widths of each cell's subcells and, if needed, updates +// cell's width to fit all subcells. static void UpdateAndPropagateRowCellWidths(AlignmentRow* node) { node->Value().UpdateWidths(); @@ -618,6 +643,7 @@ static AlignedFormattingColumnSchema ComputeColumnWidths( } } + // Make sure columns are wide enough to fit all their subcolumns for (auto& column_iter : VectorTreePostOrderTraversal(column_configs)) { if (!column_iter.is_leaf()) { int children_width = std::accumulate( From 6e32326eaf4138063f6ae5510c5902150d2b9df3 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Wed, 25 Aug 2021 16:10:08 +0200 Subject: [PATCH 13/23] Flush RHS value in ranges to the right --- verilog/formatting/align.cc | 4 ++-- verilog/formatting/formatter_test.cc | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/verilog/formatting/align.cc b/verilog/formatting/align.cc index c6edac1b1..758d6b58b 100644 --- a/verilog/formatting/align.cc +++ b/verilog/formatting/align.cc @@ -765,7 +765,7 @@ class ClassPropertyColumnSchemaScanner : public ColumnSchemaScanner { ABSL_DIE_IF_NULL(ReserveNewColumn(column, *node[1], FlushRight)); ReserveNewColumn(value_subcolumn, *node[1], FlushRight); // LHS value ReserveNewColumn(value_subcolumn, *node[2], FlushLeft); // ':' - ReserveNewColumn(value_subcolumn, *node[3], FlushLeft); // RHS value + ReserveNewColumn(value_subcolumn, *node[3], FlushRight); // RHS value ReserveNewColumn(column, *node[4], FlushLeft); // ']' return; @@ -1116,7 +1116,7 @@ class DistItemColumnSchemaScanner : public ColumnSchemaScanner { GetSubpath(Path(), {1})); // LHS value ReserveNewColumn(item_column_, *node[2], FlushLeft, GetSubpath(Path(), {2})); // ':' - ReserveNewColumn(item_column_, *node[3], FlushLeft, + ReserveNewColumn(item_column_, *node[3], FlushRight, GetSubpath(Path(), {3})); // RHS value ReserveNewColumn(item_column_, *node[4], FlushLeft, GetSubpath(Path(), {4})); // ']' diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index 22be1c1d7..017931f5a 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -3769,12 +3769,12 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { "rand bit [A:BB][42] quuz[7];\n" "endclass\n", "class cc;\n" - " rand bit [A-1:0 ] foo;\n" - " rand bit [A-1:0 ][ 2] bar;\n" + " rand bit [A-1: 0] foo;\n" + " rand bit [A-1: 0][ 2] bar;\n" " int foobar[X+1:Y];\n" " int baz [ 42];\n" " rand bit qux [ Z];\n" - " rand bit [ 1:0 ] quux [ 3:0];\n" + " rand bit [ 1: 0] quux [ 3:0];\n" " rand bit [ A:BB][42] quuz [ 7];\n" "endclass\n"}, {"class cc;\n" @@ -3990,11 +3990,11 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { "class c;\n" " constraint co {\n" " d dist {\n" - " [ 1 : 2 ] :/ 2,\n" - " [ 11 : 33 ] :/ 22,\n" + " [ 1 : 2] :/ 2,\n" + " [ 11 : 33] :/ 22,\n" " [111 : 444] :/ 8,\n" - " [ 1 : 42 ] :/ 10,\n" - " [ 11 : 12 ] :/ 3\n" + " [ 1 : 42] :/ 10,\n" + " [ 11 : 12] :/ 3\n" " };\n" " }\n" "endclass\n"}, @@ -4014,11 +4014,11 @@ static constexpr FormatterTestCase kFormatterTestCases[] = { "class foo;\n" " constraint bar {\n" " baz dist {\n" - " [ 1 : 2 ] :/ 2,\n" + " [ 1 : 2] :/ 2,\n" " QUX[3:0] :/ 10,\n" - " [ 11 : 33 ] :/ 22,\n" + " [ 11 : 33] :/ 22,\n" " ID_LONGER_THAN_RANGES :/ 3,\n" - " [ 111 : QUUZ[Z] ] :/ 8,\n" + " [ 111 : QUUZ[Z]] :/ 8,\n" " [X[4:0] : Y[8:Z-2]] :/ 8\n" " };\n" " }\n" From 4fff9f3008983dc51e09d0f87fd5f224cca0e65b Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Thu, 2 Sep 2021 01:49:46 +0200 Subject: [PATCH 14/23] Add comments --- common/formatting/align.cc | 1 + common/util/vector_tree_iterators.h | 2 ++ 2 files changed, 3 insertions(+) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index c0f0c31fb..3cf3f14f2 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -442,6 +442,7 @@ class ColumnSchemaAggregator { // CellLabelGetterFunc which creates a label with column's path relative to // its parent column and either '<' or '>' filler characters indicating whether // the column flushes to the left or the right. +// `T` should be either AggregateColumnData or ColumnPositionEntry. template static std::pair GetColumnDataCellLabel( const VectorTree& node) { diff --git a/common/util/vector_tree_iterators.h b/common/util/vector_tree_iterators.h index 5ce2b1038..be3581a48 100644 --- a/common/util/vector_tree_iterators.h +++ b/common/util/vector_tree_iterators.h @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// This header provides iterators to traverse VectorTrees in various ways. + #ifndef VERIBLE_COMMON_UTIL_VECTOR_TREE_ITERATORS_H_ #define VERIBLE_COMMON_UTIL_VECTOR_TREE_ITERATORS_H_ From e846cf608ac5be1672ff7dffa381dd1c3ca6c9ee Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Thu, 2 Sep 2021 01:50:37 +0200 Subject: [PATCH 15/23] Use explicit template types --- common/formatting/align.cc | 5 +++-- common/formatting/align_test.cc | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 3cf3f14f2..21ac9f8bd 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -447,7 +447,7 @@ template static std::pair GetColumnDataCellLabel( const VectorTree& node) { std::ostringstream label; - const auto& path = node.Value().path; + const SyntaxTreePath& path = node.Value().path; auto begin = path.begin(); if (node.Parent()) { // Find and skip common prefix @@ -461,7 +461,8 @@ static std::pair GetColumnDataCellLabel( } label << " \t "; if (begin != path.begin() && begin != path.end()) label << "."; - label << SequenceFormatter(iterator_range(begin, path.end()), "."); + label << SequenceFormatter( + iterator_range(begin, path.end()), "."); label << " \t "; return {label.str(), node.Value().properties.flush_left ? '<' : '>'}; diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index 0fc1364c0..db60c0fcd 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -989,7 +989,8 @@ class SyntaxTreeColumnizer : public ColumnSchemaScanner { else column = ReserveNewColumn(current_column_, node, props); - ValueSaver current_column_saver(¤t_column_, column); + ValueSaver current_column_saver(¤t_column_, + column); ColumnSchemaScanner::Visit(node); } void Visit(const SyntaxTreeLeaf& leaf) override { From 8c906b95eb7895ae05dd823b88515278059da5d4 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Thu, 2 Sep 2021 01:51:14 +0200 Subject: [PATCH 16/23] Iterate vector of strings using a reference --- common/formatting/align.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 21ac9f8bd..1ff5eed51 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -155,7 +155,7 @@ static std::size_t CreateTextNodes( const auto [text, filler] = get_cell_label(src_child); const std::vector lines = absl::StrSplit(text, '\n'); auto* dst_child = dst_node; - for (const auto line : lines) { + for (const auto& line : lines) { dst_child = dst_child->NewChild( Cell{line, filler, std::max(line.size(), kMinCellWidth)}); } From 2f7512aad8f45f01e845863bd92805f76e89f0e0 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Thu, 2 Sep 2021 01:51:42 +0200 Subject: [PATCH 17/23] Add deduction guides for vector tree iterator classes --- common/util/vector_tree_iterators.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/common/util/vector_tree_iterators.h b/common/util/vector_tree_iterators.h index be3581a48..3ba9abd20 100644 --- a/common/util/vector_tree_iterators.h +++ b/common/util/vector_tree_iterators.h @@ -95,6 +95,10 @@ class VectorTreeLeavesIterator } }; +template +VectorTreeLeavesIterator(VectorTreeType*) + -> VectorTreeLeavesIterator; + // Returns VectorTreeLeavesIterator range that spans all leaves of a tree. // Note that when the node passed as a tree is a leaf, the returned range spans // this node. @@ -135,6 +139,10 @@ class VectorTreePreOrderIterator } }; +template +VectorTreePreOrderIterator(VectorTreeType*) + -> VectorTreePreOrderIterator; + // Returns VectorTreePreOrderIterator range that spans all nodes of a tree // (including the tree's root). template @@ -170,6 +178,10 @@ class VectorTreePostOrderIterator this_type end() const { return this_type(_NextNode(this->node_)); } }; +template +VectorTreePostOrderIterator(VectorTreeType*) + -> VectorTreePostOrderIterator; + // Returns VectorTreePostOrderIterator range that spans all nodes of a tree // (including the tree's root). template From 5d4fec5137e0e78a4ca9d5b6616243090ee507df Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Mon, 6 Sep 2021 21:56:11 +0200 Subject: [PATCH 18/23] Add operator<<() test for VectorTree --- common/formatting/align_test.cc | 71 +++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index db60c0fcd..f14af7e80 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -1324,5 +1324,76 @@ TEST_F(InferSubcolumnsTreeAlignmentTest, InferUserIntent) { "( eleven nineteen-ninety-nine 2k )\n"); } +template +struct ColumnsTreeFormatterTestCase { + Tree input; + absl::string_view expected; +}; + +TEST(ColumnsTreeFormatter, ColumnPositionTreePrinter) { + using T = ColumnPositionTree; + using V = ColumnPositionEntry; + static const TokenInfo kFooToken(1, "foo"); + + static const ColumnsTreeFormatterTestCase kTestCases[] = { + { + T(V{{}, kFooToken, FlushLeft}), + "", + }, + { + T(V{{0, 1, 2}, kFooToken, FlushRight}), + "", + }, + { + T(V{{}, kFooToken, FlushLeft}, // + T(V{{0}, kFooToken, FlushLeft}), // + T(V{{1}, kFooToken, FlushRight}), // + T(V{{42}, kFooToken, FlushLeft})), + "| < 0 < | > 1 > | < 42 < |\n", + }, + { + T(V{{}, kFooToken, FlushLeft}, // + T(V{{0}, kFooToken, FlushLeft}), // + T(V{{1}, kFooToken, FlushRight}, // + T(V{{1, 2}, kFooToken, FlushLeft}), // + T(V{{1, 1}, kFooToken, FlushLeft}), // + T(V{{1, 3, 3}, kFooToken, FlushLeft}, // + T(V{{1, 3, 3, 1}, kFooToken, FlushLeft})), // + T(V{{2, 4, 2}, kFooToken, FlushRight}))), + "| < 0 < | >>>>>>>>>>>>>>>>> 1 >>>>>>>>>>>>>>>>>> |\n" + " | < .2 < | < .1 < | < .3.3 < | > 2.4.2 > |\n" + " | << .1 << |\n", + }, + { + T(V{{}, kFooToken, FlushLeft}, // + T(V{{0}, kFooToken, FlushLeft}), // + T(V{{1}, kFooToken, FlushLeft}), // + T(V{{42}, kFooToken, FlushLeft}, // + T(V{{3, 4, 5}, kFooToken, FlushLeft}, // not a subpath + T(V{{8}, kFooToken, FlushRight}))), // not a subpath + T(V{{2}, kFooToken, FlushLeft})), + "| < 0 < | < 1 < | << 42 <<< | < 2 < |\n" + " | < 3.4.5 < |\n" + " | >>> 8 >>> |\n", + }, + { + T(V{{}, kFooToken, FlushLeft}, // + T(V{{0}, kFooToken, FlushLeft}, // + T(V{{0, 0}, kFooToken, FlushLeft})), // + T(V{{1}, kFooToken, FlushRight}), // + T(V{{2}, kFooToken, FlushLeft}, // + T(V{{2, 0}, kFooToken, FlushLeft}))), + "| < 0 << | > 1 > | < 2 << |\n" + "| < .0 < | | < .0 < |\n", + }, + }; + + for (const auto& test_case : kTestCases) { + std::ostringstream stream; + stream << test_case.input; + EXPECT_EQ(stream.str(), test_case.expected); + } +} + } // namespace } // namespace verible From 50646b06de185cfdc95fdd34da348474977855a9 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Tue, 7 Sep 2021 23:44:15 +0200 Subject: [PATCH 19/23] Test for `contain_delimiter` column's property --- common/formatting/align_test.cc | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index f14af7e80..0bc192243 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -976,7 +976,8 @@ TEST_F(InferAmbiguousAlignIntentTest, DifferenceSufficientlySmall) { "threeeee four\n"); } -// Creates columns tree with the same layout as the syntax tree +// Creates columns tree with the same layout as the syntax tree. +// Columns created for tokens ',' have `contains_delimiter` set. template class SyntaxTreeColumnizer : public ColumnSchemaScanner { public: @@ -994,10 +995,13 @@ class SyntaxTreeColumnizer : public ColumnSchemaScanner { ColumnSchemaScanner::Visit(node); } void Visit(const SyntaxTreeLeaf& leaf) override { + AlignmentColumnProperties local_props = props; + if (leaf.get().text() == ",") local_props.contains_delimiter = true; + if (!current_column_) - ReserveNewColumn(leaf, props); + ReserveNewColumn(leaf, local_props); else - ReserveNewColumn(current_column_, leaf, props); + ReserveNewColumn(current_column_, leaf, local_props); } private: @@ -1324,6 +1328,28 @@ TEST_F(InferSubcolumnsTreeAlignmentTest, InferUserIntent) { "( eleven nineteen-ninety-nine 2k )\n"); } +class SubcolumnsTreeWithDelimitersTest : public SubcolumnsTreeAlignmentTest { + public: + SubcolumnsTreeWithDelimitersTest(absl::string_view text = + "( One Two , )\n" + "( Three Four )\n" + "\n" + "( Seven Eight , )\n" + "( Five Six )\n") + : SubcolumnsTreeAlignmentTest(text) {} +}; + +TEST_F(SubcolumnsTreeWithDelimitersTest, ContainsDelimiterTest) { + TabularAlignTokens(&partition_, kLeftAligningTreeAlignmentHandler, + &pre_format_tokens_, sample_, ByteOffsetSet(), 40); + + EXPECT_EQ(Render(), // + "(One Two,)\n" + "(ThreeFour)\n" + "(SevenEight,)\n" + "(Five Six )\n"); +} + template struct ColumnsTreeFormatterTestCase { Tree input; From b2c87b886b40684de88719faa2dc90c9d6fbf8a2 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Thu, 9 Sep 2021 23:42:22 +0200 Subject: [PATCH 20/23] Check for division by zero --- common/formatting/align.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 1ff5eed51..417559b59 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -215,9 +215,14 @@ static void ColumnsTreeFormatter( [](std::size_t width, const VectorTree& child) { return width + child.Value().width; }); + // There is at least one child; each cell minimum width is equal to: + // CreateTextNodes::kMinCellWidth + kCellSeparator.size() + CHECK_GT(children_width, 0); if (node.Value().width > children_width) { auto extra_width = node.Value().width - children_width; for (auto& child : node.Children()) { + CHECK_GT(children_width, 0); + assert(children_width > 0); const auto added_child_width = extra_width * child.Value().width / children_width; extra_width -= added_child_width; From 9130b4d131993a3d2f45de2160af12a51eb74dcf Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Fri, 10 Sep 2021 00:28:12 +0200 Subject: [PATCH 21/23] Use pointer instead of reference for non-const argument --- common/formatting/align.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/common/formatting/align.cc b/common/formatting/align.cc index 417559b59..a5193945f 100644 --- a/common/formatting/align.cc +++ b/common/formatting/align.cc @@ -372,7 +372,7 @@ static void CopyTreeStructure( class ColumnSchemaAggregator { public: void Collect(const ColumnPositionTree& columns) { - CollectColumnsTree(columns, columns_); + CollectColumnsTree(columns, &columns_); } // Sort columns by syntax tree path assigned to them and create an index that @@ -413,26 +413,27 @@ class ColumnSchemaAggregator { private: void CollectColumnsTree(const ColumnPositionTree& column, - VectorTree& aggregate_column) { + VectorTree* aggregate_column) { + CHECK_NOTNULL(aggregate_column); for (const auto& subcolumn : column.Children()) { const auto [index_entry, insert] = syntax_to_columns_map_.try_emplace(subcolumn.Value().path); VectorTree* aggregate_subcolumn; if (insert) { - aggregate_subcolumn = aggregate_column.NewChild(); + aggregate_subcolumn = aggregate_column->NewChild(); CHECK_NOTNULL(aggregate_subcolumn); // Put aggregate column node's path in created index entry aggregate_subcolumn->Path(index_entry->second); } else { // Fact: existing aggregate_subcolumn is a direct child of // aggregate_column - CHECK_GT(aggregate_column.Children().size(), + CHECK_GT(aggregate_column->Children().size(), index_entry->second.back()); aggregate_subcolumn = - &aggregate_column.Children()[index_entry->second.back()]; + &aggregate_column->Children()[index_entry->second.back()]; } aggregate_subcolumn->Value().Import(subcolumn.Value()); - CollectColumnsTree(subcolumn, *aggregate_subcolumn); + CollectColumnsTree(subcolumn, aggregate_subcolumn); } } From 4d3058f80c42778849a7aaa5673052d12b8dec37 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Fri, 10 Sep 2021 00:29:57 +0200 Subject: [PATCH 22/23] Mark single-argument constructors explicit --- common/util/vector_tree_iterators.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/common/util/vector_tree_iterators.h b/common/util/vector_tree_iterators.h index 3ba9abd20..8ccfb8a2b 100644 --- a/common/util/vector_tree_iterators.h +++ b/common/util/vector_tree_iterators.h @@ -41,7 +41,7 @@ class VectorTreeIteratorBase { using reference = VectorTreeType&; VectorTreeIteratorBase() : node_(nullptr) {} - VectorTreeIteratorBase(pointer node) : node_(node) {} + explicit VectorTreeIteratorBase(pointer node) : node_(node) {} reference operator*() const { CHECK_NOTNULL(node_); @@ -86,7 +86,7 @@ class VectorTreeLeavesIterator public: VectorTreeLeavesIterator() : base_type() {} - VectorTreeLeavesIterator(VectorTreeType* node) + explicit VectorTreeLeavesIterator(VectorTreeType* node) : base_type(node ? node->LeftmostDescendant() : nullptr) {} static VectorTreeType* _NextNode(VectorTreeType* node) { @@ -120,7 +120,7 @@ class VectorTreePreOrderIterator public: VectorTreePreOrderIterator() : base_type() {} - VectorTreePreOrderIterator(VectorTreeType* node) : base_type(node) {} + explicit VectorTreePreOrderIterator(VectorTreeType* node) : base_type(node) {} static VectorTreeType* _NextNode(VectorTreeType* node) { if (!node) return nullptr; @@ -161,7 +161,8 @@ class VectorTreePostOrderIterator public: VectorTreePostOrderIterator() : base_type() {} - VectorTreePostOrderIterator(VectorTreeType* node) : base_type(node) {} + explicit VectorTreePostOrderIterator(VectorTreeType* node) + : base_type(node) {} static VectorTreeType* _NextNode(VectorTreeType* node) { if (!node) return nullptr; From bd80d3dded47d72004f5471570df5a0f023cd823 Mon Sep 17 00:00:00 2001 From: Mariusz Glebocki Date: Fri, 10 Sep 2021 09:08:14 +0200 Subject: [PATCH 23/23] Remove redundant `.get()` --- common/formatting/align_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/formatting/align_test.cc b/common/formatting/align_test.cc index 0bc192243..0fd7990a8 100644 --- a/common/formatting/align_test.cc +++ b/common/formatting/align_test.cc @@ -1044,7 +1044,7 @@ class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture { while (true) { UnwrappedLine uwline(0, token_iter); SymbolPtr item = ParseItem(&token_iter, pre_format_tokens_.end()); - if (!item.get()) { + if (!item) { break; } uwline.SpanUpToToken(token_iter); @@ -1060,7 +1060,7 @@ class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture { const std::vector::iterator& end) { SymbolPtr list = TNode(0); SymbolPtr item; - while ((item = ParseItem(it, end)).get() != nullptr) { + while ((item = ParseItem(it, end)) != nullptr) { SymbolCastToNode(*list).AppendChild(std::move(item)); } return list;