From 88debc8e5f087e6c9c0d5249645ecbc8df24c926 Mon Sep 17 00:00:00 2001 From: David Plass Date: Wed, 8 Jan 2025 12:59:15 -0800 Subject: [PATCH] Only consider comments inside a span if they are fully inside the span; 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 https://github.com/google/xls/issues/1678 PiperOrigin-RevId: 713388090 --- xls/dslx/fmt/ast_fmt.cc | 11 ++++----- xls/dslx/fmt/ast_fmt_test.cc | 43 ++++++++++++++++++++++++++++++++++++ xls/dslx/fmt/comments.cc | 23 ++++++++++--------- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/xls/dslx/fmt/ast_fmt.cc b/xls/dslx/fmt/ast_fmt.cc index 278c3655f9..7be2206e4b 100644 --- a/xls/dslx/fmt/ast_fmt.cc +++ b/xls/dslx/fmt/ast_fmt.cc @@ -1276,15 +1276,10 @@ DocRef FmtTupleWithoutComments(const XlsTuple& n, Comments& comments, }); } -bool CommentsWithin(const Span& span, Comments& comments) { - std::vector 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. @@ -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) { @@ -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); } diff --git a/xls/dslx/fmt/ast_fmt_test.cc b/xls/dslx/fmt/ast_fmt_test.cc index 953637329a..d7be40bdbd 100644 --- a/xls/dslx/fmt/ast_fmt_test.cc +++ b/xls/dslx/fmt/ast_fmt_test.cc @@ -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 diff --git a/xls/dslx/fmt/comments.cc b/xls/dslx/fmt/comments.cc index a2582d94b8..f5277a90ab 100644 --- a/xls/dslx/fmt/comments.cc +++ b/xls/dslx/fmt/comments.cc @@ -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, @@ -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 Comments::GetComments( const Span& node_span) const { // Implementation note: this will typically be a single access (as most things