Skip to content

Commit

Permalink
Plumb left-token-context into token annotator.
Browse files Browse the repository at this point in the history 
Previously, only the right-token's context was considered.

This incurs a potentially expensive linear-time vector-copy on every leaf token
advancement during the annotation traversal.  This can be addressed by
restructuring SyntaxTreeContext in the future.

PiperOrigin-RevId: 293251778
  • Loading branch information
@fangism @hzeller
fangism authored and hzeller committed Feb 5, 2020
1 parent 81f6540 commit 35ba169
Show file tree
Hide file tree
Showing 5 changed files with 186 additions and 65 deletions.
12 changes: 11 additions & 1 deletion common/formatting/tree_annotator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ class TreeAnnotator : public TreeContextVisitor {
std::vector<PreFormatToken>::iterator next_filtered_token_;
// Pointer to end() of preformatted_tokens.
const std::vector<PreFormatToken>::iterator end_filtered_token_;

// Copy of current_context_ that is saved for use as a left-token's context
// passed into the token_annotator_ function.
SyntaxTreeContext saved_left_context_;
};

void TreeAnnotator::Annotate() {
Expand All @@ -99,15 +103,21 @@ void TreeAnnotator::CatchUpToCurrentLeaf(const TokenInfo& leaf_token) {
// so we need to compare a unique property instead of address.
// The very last token (before end_filtered_token) is an EOF token,
// which doesn't need to be annotated.
// TODO(fangism): efficiently compute greatest-common-ancestor between
// left- and right- context. Ideally this should be memo-ized as the
// traversal occurs, to avoid any unnecessary (linear) searches.
while (std::distance(next_filtered_token_, end_filtered_token_) > 1 &&
// compare const char* addresses:
next_filtered_token_->token->text.begin() != leaf_token.text.begin()) {
const auto& left_token = *next_filtered_token_;
++next_filtered_token_;
auto& right_token = *next_filtered_token_;
token_annotator_(left_token, &right_token, Context());
token_annotator_(left_token, &right_token, saved_left_context_, Context());
}
// next_filtered_token_ now points to leaf_token, now caught up.
// TODO(fangism): This costs an entire vector/stack-copy for every leaf token.
// May need to choose a different structure for SyntaxTreeContext.
saved_left_context_ = Context();
}

} // namespace
Expand Down
9 changes: 6 additions & 3 deletions common/formatting/tree_annotator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@

namespace verible {

// Parameters: left token, right token (modified), syntax tree context
using ContextTokenAnnotatorFunction = std::function<void(
const PreFormatToken&, PreFormatToken*, const SyntaxTreeContext&)>;
// Parameters: left token, right token (modified),
// left token's syntax tree context,
// right token's syntax tree context.
using ContextTokenAnnotatorFunction =
std::function<void(const PreFormatToken&, PreFormatToken*,
const SyntaxTreeContext&, const SyntaxTreeContext&)>;

// Applies inter-token formatting annotations, using syntactic context
// at every token.
Expand Down
114 changes: 105 additions & 9 deletions common/formatting/tree_annotator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,19 @@
namespace verible {
namespace {

void DoNothing(const PreFormatToken&, PreFormatToken*,
const SyntaxTreeContext&) {}
using ::testing::ElementsAre;

std::vector<int> ExtractSyntaxTreeContextEnums(
const SyntaxTreeContext& context) {
std::vector<int> result;
for (const auto* node : context) {
result.push_back(ABSL_DIE_IF_NULL(node)->Tag().tag);
}
return result;
}

static void DoNothing(const PreFormatToken&, PreFormatToken*,
const SyntaxTreeContext&, const SyntaxTreeContext&) {}

TEST(AnnotateFormatTokensUsingSyntaxContextTest, EmptyFormatTokens) {
std::vector<PreFormatToken> ftokens;
Expand All @@ -39,7 +50,8 @@ TEST(AnnotateFormatTokensUsingSyntaxContextTest, EmptyFormatTokens) {
constexpr int kForcedSpaces = 5;

void ForceSpaces(const PreFormatToken&, PreFormatToken* right,
const SyntaxTreeContext&) {
const SyntaxTreeContext& /* left_context */,
const SyntaxTreeContext& /* right_context */) {
right->before.spaces_required = kForcedSpaces;
}

Expand Down Expand Up @@ -69,7 +81,8 @@ TEST(AnnotateFormatTokensUsingSyntaxContextTest, UnusedContext) {
}

void LeftIsB(const PreFormatToken& left, PreFormatToken* right,
const SyntaxTreeContext&) {
const SyntaxTreeContext& /* left_context */,
const SyntaxTreeContext& /* right_context */) {
if (left.token->text == "b") {
right->before.spaces_required = kForcedSpaces;
} else {
Expand All @@ -95,16 +108,18 @@ TEST(AnnotateFormatTokensUsingSyntaxContextTest, UnusedContextBasedOnLeft) {
EXPECT_EQ(ftokens[2].before.spaces_required, kForcedSpaces);
}

void ContextDirectParentIsNine(const PreFormatToken&, PreFormatToken* right,
const SyntaxTreeContext& context) {
if (context.DirectParentIs(9)) {
void RightContextDirectParentIsNine(const PreFormatToken&,
PreFormatToken* right,
const SyntaxTreeContext& left_context,
const SyntaxTreeContext& right_context) {
if (right_context.DirectParentIs(9)) {
right->before.spaces_required = kForcedSpaces;
} else {
right->before.spaces_required = kForcedSpaces + 2;
}
}

TEST(AnnotateFormatTokensUsingSyntaxContextTest, UsingContext) {
TEST(AnnotateFormatTokensUsingSyntaxContextTest, UsingRightContext) {
const absl::string_view text("abc");
const TokenInfo tokens[] = {
{4, text.substr(0, 1)},
Expand All @@ -123,10 +138,91 @@ TEST(AnnotateFormatTokensUsingSyntaxContextTest, UsingContext) {
);
AnnotateFormatTokensUsingSyntaxContext(&*tree, tokens[3], ftokens.begin(),
ftokens.end(),
ContextDirectParentIsNine);
RightContextDirectParentIsNine);
EXPECT_EQ(ftokens[1].before.spaces_required, kForcedSpaces + 2);
EXPECT_EQ(ftokens[2].before.spaces_required, kForcedSpaces);
}

void LeftContextDirectParentIsSeven(const PreFormatToken&,
PreFormatToken* right,
const SyntaxTreeContext& left_context,
const SyntaxTreeContext& right_context) {
if (left_context.DirectParentIs(7)) {
right->before.spaces_required = kForcedSpaces + 4;
} else {
right->before.spaces_required = kForcedSpaces;
}
}

TEST(AnnotateFormatTokensUsingSyntaxContextTest, UsingLeftContext) {
const absl::string_view text("abc");
const TokenInfo tokens[] = {
{4, text.substr(0, 1)},
{5, text.substr(1, 1)},
{6, text.substr(2, 1)},
{verible::TK_EOF, text.substr(3, 0)}, // EOF
};
std::vector<PreFormatToken> ftokens;
for (const auto& t : tokens) {
ftokens.emplace_back(&t);
}
const auto tree = TNode(6, // synthesized syntax tree
TNode(7, Leaf(tokens[0])), //
TNode(8, Leaf(tokens[1])), //
TNode(9, Leaf(tokens[2])) //
);
AnnotateFormatTokensUsingSyntaxContext(&*tree, tokens[3], ftokens.begin(),
ftokens.end(),
LeftContextDirectParentIsSeven);
EXPECT_EQ(ftokens[1].before.spaces_required, kForcedSpaces + 4);
EXPECT_EQ(ftokens[2].before.spaces_required, kForcedSpaces);
}

TEST(AnnotateFormatTokensUsingSyntaxContextTest, VerifySlidingContexts) {
const absl::string_view text("abcdefgh");
const TokenInfo tokens[] = {
{4, text.substr(0, 1)}, {5, text.substr(1, 1)},
{6, text.substr(2, 1)}, {4, text.substr(3, 1)},
{5, text.substr(4, 1)}, {6, text.substr(5, 1)},
{4, text.substr(6, 1)}, {verible::TK_EOF, text.substr(7, 0)}, // EOF
};
const auto tree = TNode(6, // synthesized syntax tree
TNode(7, //
Leaf(tokens[0]), //
TNode(10, //
Leaf(tokens[1]), //
TNode(12)), //
TNode(11, //
Leaf(tokens[2]), //
Leaf(tokens[3])) //
), //
TNode(8, //
Leaf(tokens[4]), //
Leaf(tokens[5])), //
TNode(9, Leaf(tokens[6])) //
);
std::vector<PreFormatToken> ftokens;
for (const auto& t : tokens) {
ftokens.emplace_back(&t);
}
std::vector<std::vector<int>> saved_contexts;
saved_contexts.push_back({6, 7}); // first leaf's context
auto context_listener = [&](const PreFormatToken&, PreFormatToken*,
const SyntaxTreeContext& left_context,
const SyntaxTreeContext& right_context) {
// continuity and consistency check
const auto left_enums = ExtractSyntaxTreeContextEnums(left_context);
EXPECT_EQ(left_enums, saved_contexts.back());
saved_contexts.push_back(ExtractSyntaxTreeContextEnums(right_context));
};
AnnotateFormatTokensUsingSyntaxContext(&*tree, tokens[7], ftokens.begin(),
ftokens.end(), context_listener);
using V = std::vector<int>;
EXPECT_THAT(saved_contexts,
ElementsAre( //
V({6, 7}), V({6, 7, 10}), V({6, 7, 11}), V({6, 7, 11}),
V({6, 8}), V({6, 8}), V({6, 9}), V()));
}

} // namespace
} // namespace verible
Loading

0 comments on commit 35ba169

Please sign in to comment.