Skip to content

Commit

Permalink
Only consider comments inside a span if they are fully inside the spa…
Browse files Browse the repository at this point in the history
…n; previously they were considered "in the span" if their line number was in the span.

This was causing issues when formatting short blocks with comments, especially in tuples:

`if bar > u32:0 { u1:1} else {1:0} // this comment will be repeated`

which was erroneously formatted to:

```
    if bar > u32:0 {
        u1:1  // this comment will be repeated
    } else {
        u1:0  // this comment will be repeated
    } // this comment will be repeated
```

Fixes #1678

PiperOrigin-RevId: 713388090
  • Loading branch information
dplassgit authored and copybara-github committed Jan 8, 2025
1 parent 085321c commit 88debc8
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 17 deletions.
11 changes: 4 additions & 7 deletions xls/dslx/fmt/ast_fmt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1276,15 +1276,10 @@ DocRef FmtTupleWithoutComments(const XlsTuple& n, Comments& comments,
});
}

bool CommentsWithin(const Span& span, Comments& comments) {
std::vector<const CommentData*> items = comments.GetComments(span);
return !items.empty();
}

DocRef FmtTuple(const XlsTuple& n, Comments& comments, DocArena& arena) {
Span tuple_span = n.span();
// Detect if there are any comments in the span of the tuple.
bool any_comments = CommentsWithin(tuple_span, comments);
bool any_comments = comments.HasComments(tuple_span);

if (!any_comments) {
// Do it the old way.
Expand Down Expand Up @@ -1314,6 +1309,8 @@ DocRef FmtTuple(const XlsTuple& n, Comments& comments, DocArena& arena) {
pieces.push_back(arena.comma());
pieces.push_back(arena.space());
}
// TODO: davidplass - if the previous comment is not on the same line as
// the previous element, insert a hard line before the comment.
pieces.push_back(previous_comments.value());
pieces.push_back(arena.hard_line());
} else if (!first_element) {
Expand Down Expand Up @@ -1503,7 +1500,7 @@ DocRef Fmt(const StructInstance& n, Comments& comments, DocArena& arena) {
// If there are comments within the span, we must go to break mode, because
// newlines.
DocRef on_break = FmtBreakRest(n, comments, arena);
if (CommentsWithin(n.span(), comments)) {
if (comments.HasComments(n.span())) {
return arena.MakeConcat(leader, on_break);
}

Expand Down
43 changes: 43 additions & 0 deletions xls/dslx/fmt/ast_fmt_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3343,5 +3343,48 @@ struct Foo { a: u1, b: u1 }
)");
}

TEST_F(ModuleFmtTest, TupleWithComment_GH_1678) {
DoFmt(R"(fn foo(bar: u32) {
let some_data_to_make_single_update_per_line = u32:0xabcdef;
(
bit_slice_update(
some_data_to_make_single_update_per_line, 1,
if bar > u32:0xdeadbeef { u1:1 } else { u1:0 }), // this is yet another comment
)
}
)");
}

TEST_F(ModuleFmtTest, TupleWithMultipleComments_GH_1678) {
DoFmt(R"(fn foo(bar: u32) {
let some_data_to_make_single_update_per_line = u32:0xabcdef;
(
// TODO: davidplass - if the previous comment is not on the same line as
// the previous element, insert a hard line before the comment.
bit_slice_update(some_data_to_make_single_update_per_line, 0, u1:1),
// This is an important comment
bit_slice_update(
some_data_to_make_single_update_per_line, 1,
if bar > u32:0xdeadbeef { u1:1 } else { u1:0 }), // this is yet another comment
bit_slice_update(some_data_to_make_single_update_per_line, 2, u1:1)
)
}
)",
R"(fn foo(bar: u32) {
let some_data_to_make_single_update_per_line = u32:0xabcdef;
(
// TODO: davidplass - if the previous comment is not on the same line as
// the previous element, insert a hard line before the comment.
bit_slice_update(some_data_to_make_single_update_per_line, 0, u1:1), // This is an important
// comment
bit_slice_update(
some_data_to_make_single_update_per_line, 1,
if bar > u32:0xdeadbeef { u1:1 } else { u1:0 }), // this is yet another comment
bit_slice_update(some_data_to_make_single_update_per_line, 2, u1:1)
)
}
)");
}

} // namespace
} // namespace xls::dslx
23 changes: 13 additions & 10 deletions xls/dslx/fmt/comments.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,6 @@ namespace xls::dslx {
return Comments{std::move(line_to_comment), last_data_limit};
}

bool Comments::HasComments(const Span& in_span) const {
for (int64_t i = in_span.start().lineno(); i <= in_span.limit().lineno();
++i) {
if (auto it = line_to_comment_.find(i); it != line_to_comment_.end()) {
return true;
}
}
return false;
}

static bool InRange(const Span& node_span, const CommentData& comment) {
// For multiline comments, consider in range if the comment start is within
// the node span. Since all comments end on the line below the comment,
Expand All @@ -66,6 +56,19 @@ static bool InRange(const Span& node_span, const CommentData& comment) {
return overlapping_multiline || node_span.Contains(comment.span);
}

bool Comments::HasComments(const Span& in_span) const {
for (int64_t i = in_span.start().lineno(); i <= in_span.limit().lineno();
++i) {
if (auto it = line_to_comment_.find(i); it != line_to_comment_.end()) {
const CommentData& cd = it->second;
if (InRange(in_span, cd)) {
return true;
}
}
}
return false;
}

std::vector<const CommentData*> Comments::GetComments(
const Span& node_span) const {
// Implementation note: this will typically be a single access (as most things
Expand Down

0 comments on commit 88debc8

Please sign in to comment.