Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] TokenPartitionTree refactoring / "Choice" nodes support #1328

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

[WIP] TokenPartitionTree refactoring / "Choice" nodes support #1328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
991980f
[WIP] Add Variant class.
Apr 29, 2022
2155dc3
[WIP] Tree Operations: Add support for Children() returning a pointer.
Apr 29, 2022
99860e7
Add FlattenOneChild overload taking iterator.
Apr 29, 2022
7830635
Add is_detected_v.
Apr 30, 2022
d539734
[WIP] Tree Operations: More compile-time type checks.
Apr 30, 2022
25d8f4b
Add debug helper for printing readable C++ types.
Apr 30, 2022
392ca74
UnwrappedLine: Reorder data members to decrease object size.
Apr 30, 2022
12fcbac
[WIP] Convert TokenPartitionTree into class hierarchy wrapper.
Apr 30, 2022
cb3a29d
Make clang-tidy happy: use ptr instead of ref.
May 9, 2022
48b46d7
Add deduction guides for ValueSaver
May 9, 2022
40644fe
Refactor TokenPartitionTreePrinter
May 9, 2022
d2e94bd
Overload stream printer for TokenPartitionTree
May 9, 2022
806ba2e
Use enable_if_t in a form understandable for clang-tidy.
May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions common/formatting/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,10 @@ cc_library(
"//common/util:spacer",
"//common/util:top_n",
"//common/util:tree_operations",
"//common/util:value_saver",
"//common/util:variant",
"//common/util:vector_tree",
"@com_google_absl//absl/strings",
],
)

Expand Down
28 changes: 14 additions & 14 deletions common/formatting/align.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ void AlignablePartitionGroup::ApplyAlignment(
const GroupAlignmentData& align_data) const {
auto row = alignable_rows_.begin();
for (const auto& align_actions : align_data.align_actions_2D) {
(*row)->Children().clear();
(*row)->Children()->clear();
VLOG(3) << __FUNCTION__ << " processing row: " << **row;
if (!align_actions.empty()) {
auto& node = **row;
Expand All @@ -994,9 +994,9 @@ void AlignablePartitionGroup::ApplyAlignment(

verible::TokenPartitionTree* current_cell = nullptr;
if (align_actions.front().ftoken != ftokens.begin()) {
node.Children().emplace_back(
node.Children()->emplace_back(
UnwrappedLine(0, ftokens.begin(), PartitionPolicyEnum::kInline));
current_cell = &node.Children().back();
current_cell = &node.Children()->back();
}

for (const auto& action : align_actions) {
Expand All @@ -1007,10 +1007,10 @@ void AlignablePartitionGroup::ApplyAlignment(
<< StringSpanOfTokenRange(current_cell->Value().TokensRange())
<< " ]";
}
node.Children().emplace_back(
node.Children()->emplace_back(
UnwrappedLine(action.new_before_spacing, action.ftoken,
PartitionPolicyEnum::kInline));
current_cell = &node.Children().back();
current_cell = &node.Children()->back();
}
if (current_cell) {
current_cell->Value().SpanUpToToken(ftokens.end());
Expand Down Expand Up @@ -1140,7 +1140,7 @@ void FormatUsingOriginalSpacing(TokenPartitionRange partition_range) {
VLOG(4) << "partition before:\n"
<< TokenPartitionTreePrinter(partition, true);

partition.Children().clear();
partition.Children()->clear();
auto tokens = partition.Value().TokensRange();
if (tokens.empty()) {
partition.Value().SetPartitionPolicy(
Expand All @@ -1157,7 +1157,7 @@ void FormatUsingOriginalSpacing(TokenPartitionRange partition_range) {

auto line = UnwrappedLine(indentation, tokens.begin(),
PartitionPolicyEnum::kAlreadyFormatted);
partition.Children().emplace_back(line);
partition.Children()->emplace_back(line);

if (tokens.size() > 1) {
// First token
Expand All @@ -1167,7 +1167,7 @@ void FormatUsingOriginalSpacing(TokenPartitionRange partition_range) {
auto slice =
UnwrappedLine(0, tokens.begin(), PartitionPolicyEnum::kInline);
slice.SpanNextToken();
partition.Children().back().Children().emplace_back(slice);
partition.Children()->back().Children()->emplace_back(slice);

// Remaining tokens
for (auto it = tokens.begin() + 1; it != tokens.end(); ++it) {
Expand All @@ -1180,7 +1180,7 @@ void FormatUsingOriginalSpacing(TokenPartitionRange partition_range) {
std::size_t last_newline_pos = whitespace.find_last_of('\n');
if (last_newline_pos != absl::string_view::npos) {
// Update end of current line.
partition.Children().back().Value().SpanUpToToken(it);
partition.Children()->back().Value().SpanUpToToken(it);
// Start a new line.
// Newlines count does not matter here. All newlines in leading
// whitespace of the first token in a line are always preserved.
Expand All @@ -1192,19 +1192,19 @@ void FormatUsingOriginalSpacing(TokenPartitionRange partition_range) {
// indentation + (this line orig. indent) - (1st line orig. indent)
const auto line =
UnwrappedLine(0, it, PartitionPolicyEnum::kAlreadyFormatted);
partition.Children().emplace_back(line);
partition.Children()->emplace_back(line);
// Count only spaces after the last '\n'.
spacing -= last_newline_pos + 1;
}

auto slice = UnwrappedLine(spacing, it, PartitionPolicyEnum::kInline);
slice.SpanNextToken();
partition.Children().back().Children().emplace_back(slice);
partition.Children()->back().Children()->emplace_back(slice);
}
}
partition.Children().back().Value().SpanUpToToken(tokens.end());
partition.Children()->back().Value().SpanUpToToken(tokens.end());

if (partition.Children().size() == 1) {
if (partition.Children()->size() == 1) {
HoistOnlyChild(partition);
} else {
partition.Value().SetPartitionPolicy(PartitionPolicyEnum::kAlwaysExpand);
Expand Down Expand Up @@ -1271,7 +1271,7 @@ void TabularAlignTokens(
// possibly some other ignored element like comments.

auto& partition = *partition_ptr;
auto& subpartitions = partition.Children();
auto& subpartitions = *partition.Children();
// Identify groups of partitions to align, separated by blank lines.
const TokenPartitionRange subpartitions_range(subpartitions.begin(),
subpartitions.end());
Expand Down
30 changes: 15 additions & 15 deletions common/formatting/align_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ class MatrixTreeAlignmentTestFixture : public AlignmentTestFixture {

std::string Render() {
std::ostringstream stream;
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
const auto policy = child.Value().PartitionPolicy();
if (policy == PartitionPolicyEnum::kAlreadyFormatted) {
ApplyAlreadyFormattedPartitionPropertiesToTokens(&child,
Expand Down Expand Up @@ -333,7 +333,7 @@ TEST_F(Sparse3x3MatrixAlignmentTest, OneInterTokenPaddingWithIndent) {
ftoken.before.spaces_required = 1;
}
// Indent each partition.
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
child.Value().SetIndentationSpaces(4);
}

Expand All @@ -354,7 +354,7 @@ TEST_F(Sparse3x3MatrixAlignmentTest, IgnoreCommentLine) {
}
// Leave the 'commented' line indented.
pre_format_tokens_[2].before.break_decision = SpacingOptions::MustWrap;
partition_.Children()[1].Value().SetIndentationSpaces(1);
partition_.Children()->at(1).Value().SetIndentationSpaces(1);

// Pretend lines that begin with "three" are to be ignored, like comments.
auto ignore_threes = [](const TokenPartitionTree& partition) {
Expand Down Expand Up @@ -406,7 +406,7 @@ TEST_F(Sparse3x3MatrixAlignmentTest, CompletelyDisabledNoAlignmentWithIndent) {
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
child.Value().SetIndentationSpaces(3);
}
pre_format_tokens_[0].before.break_decision = SpacingOptions::MustWrap;
Expand Down Expand Up @@ -442,7 +442,7 @@ TEST_F(Sparse3x3MatrixAlignmentMoreSpacesTest,
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
child.Value().SetIndentationSpaces(1);
}
pre_format_tokens_[0].before.break_decision = SpacingOptions::MustWrap;
Expand Down Expand Up @@ -510,7 +510,7 @@ TEST_F(Sparse3x3MatrixAlignmentTest, DisabledByColumnLimitIndented) {
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
child.Value().SetIndentationSpaces(3);
}

Expand Down Expand Up @@ -592,7 +592,7 @@ class MultiAlignmentGroupTest : public AlignmentTestFixture {
std::ostringstream stream;
int position = 0;
const absl::string_view text(sample_);
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
const auto policy = child.Value().PartitionPolicy();
if (policy == PartitionPolicyEnum::kAlreadyFormatted) {
ApplyAlreadyFormattedPartitionPropertiesToTokens(&child,
Expand Down Expand Up @@ -717,8 +717,8 @@ class GetPartitionAlignmentSubrangesTestFixture : public AlignmentTestFixture {
};

TEST_F(GetPartitionAlignmentSubrangesTestFixture, VariousRanges) {
const TokenPartitionRange children(partition_.Children().begin(),
partition_.Children().end());
const TokenPartitionRange children(partition_.Children()->begin(),
partition_.Children()->end());

const std::vector<TaggedTokenPartitionRange> ranges(
GetPartitionAlignmentSubranges(children, [](const TokenPartitionTree&
Expand Down Expand Up @@ -820,8 +820,8 @@ class GetPartitionAlignmentSubrangesSubtypedTestFixture
};

TEST_F(GetPartitionAlignmentSubrangesSubtypedTestFixture, VariousRanges) {
const TokenPartitionRange children(partition_.Children().begin(),
partition_.Children().end());
const TokenPartitionRange children(partition_.Children()->begin(),
partition_.Children()->end());

const std::vector<TaggedTokenPartitionRange> ranges(
GetPartitionAlignmentSubranges(children, &PartitionSelector));
Expand Down Expand Up @@ -1058,7 +1058,7 @@ class SubcolumnsTreeAlignmentTest : public MatrixTreeAlignmentTestFixture {
}
uwline.SpanUpToToken(token_iter);
uwline.SetOrigin(item.get());
partition_.Children().emplace_back(std::move(uwline));
partition_.Children()->emplace_back(std::move(uwline));
SymbolCastToNode(*syntax_tree_).AppendChild(std::move(item));
}
}
Expand Down Expand Up @@ -1185,7 +1185,7 @@ TEST_F(SubcolumnsTreeAlignmentTest, OneInterTokenPaddingExceptFront) {
ftoken.before.spaces_required = 1;
}
// Find first token of each line and require 0 spaces before them.
for (auto& line : partition_.Children()) {
for (auto& line : *partition_.Children()) {
const auto tokens = line.Value().TokensRange();
if (!tokens.empty()) {
const PreFormatToken& front = tokens.front();
Expand Down Expand Up @@ -1232,7 +1232,7 @@ TEST_F(SubcolumnsTreeAlignmentTest,
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
for (auto& line : partition_.Children()) {
for (auto& line : *partition_.Children()) {
line.Value().SetIndentationSpaces(2);
}

Expand Down Expand Up @@ -1262,7 +1262,7 @@ class MultiSubcolumnsTreeAlignmentTest : public SubcolumnsTreeAlignmentTest {
std::ostringstream stream;
int position = 0;
const absl::string_view text(sample_);
for (auto& child : partition_.Children()) {
for (auto& child : *partition_.Children()) {
const auto policy = child.Value().PartitionPolicy();
if (policy == PartitionPolicyEnum::kAlreadyFormatted) {
ApplyAlreadyFormattedPartitionPropertiesToTokens(&child,
Expand Down
58 changes: 29 additions & 29 deletions common/formatting/layout_optimizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ int AlreadyFormattedPartitionLength(const TokenPartitionTree& partition) {
width += token.before.spaces_required + token.Length();
}

for (const auto& child : partition.Children()) {
for (const auto& child : *partition.Children()) {
CHECK_EQ(child.Value().PartitionPolicy(), PartitionPolicyEnum::kInline);
if (child.Value().TokensRange().begin() != tokens.begin()) {
const auto& first_token = child.Value().TokensRange().front();
Expand Down Expand Up @@ -500,7 +500,7 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(

// Traverse and calculate children layouts

absl::FixedArray<LayoutFunction> layouts(node.Children().size());
absl::FixedArray<LayoutFunction> layouts(node.Children()->size());

switch (node.Value().PartitionPolicy()) {
case PartitionPolicyEnum::kJuxtaposition:
Expand All @@ -509,7 +509,7 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
case PartitionPolicyEnum::kFitOnLineElseExpand:
case PartitionPolicyEnum::kAppendFittingSubPartitions:
case PartitionPolicyEnum::kJuxtapositionOrIndentedStack: {
std::transform(node.Children().begin(), node.Children().end(),
std::transform(node.Children()->begin(), node.Children()->end(),
layouts.begin(), [=](const TokenPartitionTree& n) {
return this->CalculateOptimalLayout(n);
});
Expand All @@ -520,7 +520,7 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
case PartitionPolicyEnum::kAlwaysExpand:
case PartitionPolicyEnum::kTabularAlignment: {
const int indentation = node.Value().IndentationSpaces();
std::transform(node.Children().begin(), node.Children().end(),
std::transform(node.Children()->begin(), node.Children()->end(),
layouts.begin(), [=](const TokenPartitionTree& n) {
const int relative_indentation =
n.Value().IndentationSpaces() - indentation;
Expand Down Expand Up @@ -558,10 +558,11 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
case PartitionPolicyEnum::kStack:
return factory_.Stack(layouts.begin(), layouts.end());
case PartitionPolicyEnum::kWrap: {
if (VLOG_IS_ON(0) && node.Children().size() > 2) {
const int indentation = node.Children()[1].Value().IndentationSpaces();
for (const auto& child : iterator_range(node.Children().begin() + 2,
node.Children().end())) {
if (VLOG_IS_ON(0) && node.Children()->size() > 2) {
const int indentation =
node.Children()->at(1).Value().IndentationSpaces();
for (const auto& child : iterator_range(node.Children()->begin() + 2,
node.Children()->end())) {
if (child.Value().IndentationSpaces() != indentation) {
VLOG(0) << "Indentations of subpartitions from the second to the "
"last are not equal. Using indentation of the second "
Expand All @@ -571,8 +572,8 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
}
}
const int hanging_indentation =
(node.Children().size() > 1)
? (node.Children()[1].Value().IndentationSpaces() -
(node.Children()->size() > 1)
? (node.Children()->at(1).Value().IndentationSpaces() -
node.Value().IndentationSpaces())
: 0;

Expand All @@ -592,7 +593,7 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
const int indentation = node.Value().IndentationSpaces();
for (size_t i = 0; i < layouts.size(); ++i) {
const int relative_indentation =
node.Children()[i].Value().IndentationSpaces() - indentation;
node.Children()->at(i).Value().IndentationSpaces() - indentation;
layouts[i] = factory_.Indent(layouts[i], relative_indentation);
}
auto stack = factory_.Stack(layouts.begin(), layouts.end());
Expand All @@ -615,7 +616,7 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
// When not a leaf, it contains partitions with kInline
// policy. Pack them horizontally.
const bool all_children_are_inlines =
std::all_of(node.Children().begin(), node.Children().end(),
std::all_of(node.Children()->begin(), node.Children()->end(),
[](const TokenPartitionTree& child) {
return child.Value().PartitionPolicy() ==
PartitionPolicyEnum::kInline;
Expand All @@ -631,7 +632,7 @@ LayoutFunction TokenPartitionsLayoutOptimizer::CalculateOptimalLayout(
// Preserve spacing of the first sublayout. This has to be done because
// the first layout in a line uses IndentationSpaces instead of
// SpacesBefore.
const auto indent = node.Children().front().Value().IndentationSpaces();
const auto indent = node.Children()->front().Value().IndentationSpaces();
layouts.front() = factory_.Indent(layouts.front(), indent);

return factory_.Juxtaposition(layouts.begin(), layouts.end());
Expand Down Expand Up @@ -679,26 +680,26 @@ void TreeReconstructor::TraverseTree(const LayoutTree& layout_tree) {
layout.TokensRange().begin(),
PartitionPolicyEnum::kAlreadyFormatted);
uwline.SpanUpToToken(layout.TokensRange().end());
tree_.Children().emplace_back(uwline);
current_node_ = &tree_.Children().back();
tree_.Children()->emplace_back(uwline);
current_node_ = &tree_.Children()->back();
} else {
const auto tokens = layout.TokensRange();
CHECK(current_node_->Value().TokensRange().end() == tokens.begin());

current_node_->Value().SpanUpToToken(tokens.end());

auto& slices = current_node_->Children();
auto& slices = *current_node_->Children();
// TODO(mglb): add support for break_decision == Preserve
if (layout.SpacesBefore() == tokens.front().before.spaces_required) {
// No need for separate inline partition
if (!slices.empty())
if (!is_leaf(*current_node_))
slices.back().Value().SpanUpToToken(tokens.end());
return;
}

// Wrap previous tokens in the line
if (slices.empty()) {
current_node_->Children().emplace_back(
if (is_leaf(*current_node_)) {
current_node_->Children()->emplace_back(
UnwrappedLine(0, current_node_->Value().TokensRange().begin(),
PartitionPolicyEnum::kInline));
}
Expand All @@ -708,7 +709,7 @@ void TreeReconstructor::TraverseTree(const LayoutTree& layout_tree) {
auto slice = UnwrappedLine(layout.SpacesBefore(), tokens.begin(),
PartitionPolicyEnum::kInline);
slice.SpanUpToToken(tokens.end());
current_node_->Children().emplace_back(slice);
current_node_->Children()->emplace_back(slice);
}
return;
}
Expand Down Expand Up @@ -756,20 +757,19 @@ void TreeReconstructor::TraverseTree(const LayoutTree& layout_tree) {
void TreeReconstructor::ReplaceTokenPartitionTreeNode(
TokenPartitionTree* node) {
CHECK_NOTNULL(node);
CHECK(!tree_.Children().empty());
CHECK(!is_leaf(tree_));

if (tree_.Children().size() == 1) {
*node = std::move(tree_.Children().front());
if (tree_.Children()->size() == 1) {
*node = std::move(tree_.Children()->front());
} else {
const auto& first_line = tree_.Children().front().Value();
const auto& last_line = tree_.Children().back().Value();
const auto& first_line = tree_.Children()->front().Value();
const auto& last_line = tree_.Children()->back().Value();

node->Value() = UnwrappedLine(current_indentation_spaces_,
tree_.Value() = UnwrappedLine(current_indentation_spaces_,
first_line.TokensRange().begin(),
PartitionPolicyEnum::kAlwaysExpand);
node->Value().SpanUpToToken(last_line.TokensRange().end());
node->Children().clear();
AdoptSubtreesFrom(*node, &tree_);
tree_.Value().SpanUpToToken(last_line.TokensRange().end());
*node = std::move(tree_);
}
}

Expand Down
Loading