Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 12 additions & 0 deletions verible/common/formatting/align.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,18 @@ void AlignablePartitionGroup::ApplyAlignment(
auto &line = node.Value();
auto ftokens = line.TokensRange();

// Leading non-tree tokens before a forced wrap must stay on their own
// line. Putting them in a kInline prolog cell would glue e.g. `//\` onto
// the following `input` (GitHub issue 2539). Preserve original spacing
// for the whole row instead.
if (align_actions.front().ftoken != ftokens.begin() &&
align_actions.front().ftoken->before.break_decision ==
SpacingOptions::kMustWrap) {
FormatUsingOriginalSpacing(TokenPartitionRange(*row, std::next(*row)));
++row;
continue;
}

line.SetPartitionPolicy(PartitionPolicyEnum::kAlreadyFormatted);

verible::TokenPartitionTree *current_cell = nullptr;
Expand Down
29 changes: 17 additions & 12 deletions verible/common/formatting/align.h
Original file line number Diff line number Diff line change
Expand Up @@ -410,19 +410,24 @@ ColumnPositionTree ScanPartitionForAlignmentCells_WithNonTreeTokens(
// Use next token as begining of trailing non-tree tokens
trailing_tokens.set_begin(ftoken_it + 1);

// Breaking following condition leads to e.g. concatenation of EOL comment
// and code in a single line. To fix situation that lead to this, flatten
// token partitions that contain EOL comment subpartition just before a
// subpartition that starts with the same token as Origin(). Example of a
// partition that needs flattening:
// Leading non-tree tokens (e.g. // comments, line-continuation `\`) cannot
// be placed into alignment cells when the first syntax-tree token must
// start a new line (SpacingOptions::kMustWrap). Including them would glue
// the leading tokens onto the origin line via kInline cells.
//
// { (>>[...], (origin: "input bit second"))
// { (>>[// comment] }
// { (>>[input bit second], (origin: "input")) }
// }
CHECK(leading_tokens.empty() || first_tree_token_it == ftokens.end() ||
first_tree_token_it->before.break_decision !=
SpacingOptions::kMustWrap);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already addressed in
#2530 so creates a merge conflict here.

It is solved there slightly differently (just returning column entries) so double check and settle on the best solution

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm my test still fails after #2530. I will therefore merge and refactor.

// This shape often appears when a `//` comment is followed by `\` (line
// continuation) and is partitioned with the following port/declaration
// (GitHub issue 2539). Prefer leaving leading tokens out of alignment
// over CHECK-failing. Callers should also ignore such partitions (see
// PartitionHasLeadingTokensBeforeForcedWrap) so ApplyAlignment never
// builds a kInline prolog for them. A future improvement could split the
// comment into its own ignored row so the following declaration can still
// participate in alignment.
if (!leading_tokens.empty() && first_tree_token_it != ftokens.end() &&
first_tree_token_it->before.break_decision ==
SpacingOptions::kMustWrap) {
leading_tokens.set_end(leading_tokens.begin());
}
} else {
// All tokens are passed as leading
leading_tokens.set_end(ftokens.end());
Expand Down
30 changes: 30 additions & 0 deletions verible/verilog/formatting/align.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,32 @@ static bool SeparatorCommentsBreakGroups(AlignmentGroupBoundary b) {
b == AlignmentGroupBoundary::kBlankLinesAndSeparatorComments;
}

// True when non-tree tokens (e.g. // comments, line-continuation `\`) precede
// the origin, and the first origin token must start a new line. Aligning such
// partitions would glue the leading tokens onto the origin line via kInline
// cells (GitHub issue 2539). Leave them out of alignment instead.
static bool PartitionHasLeadingTokensBeforeForcedWrap(
const TokenPartitionTree &partition) {
const auto &uwline = partition.Value();
const verible::Symbol *origin = uwline.Origin();
if (origin == nullptr) return false;

const auto ftokens = uwline.TokensRange();
if (ftokens.empty()) return false;

const verible::SyntaxTreeLeaf *first_leaf = verible::GetLeftmostLeaf(*origin);
if (first_leaf == nullptr) return false;

const verible::TokenInfo &first_tree_token = first_leaf->get();
auto ftoken_it = ftokens.begin();
while (ftoken_it != ftokens.end() &&
*(ftoken_it->token) != first_tree_token) {
++ftoken_it;
}
if (ftoken_it == ftokens.begin() || ftoken_it == ftokens.end()) return false;
return ftoken_it->before.break_decision == verible::SpacingOptions::kMustWrap;
}

static bool IgnoreCommentsAndPreprocessingDirectives(
const TokenPartitionTree &partition) {
const auto &uwline = partition.Value();
Expand All @@ -159,6 +185,8 @@ static bool IgnoreCommentsAndPreprocessingDirectives(
// ignore lines containing only comments
if (TokensAreAllCommentsOrAttributes(token_range)) return true;

if (PartitionHasLeadingTokensBeforeForcedWrap(partition)) return true;

// ignore partitions belonging to preprocessing directives
return IsPreprocessorKeyword(
verilog_tokentype(token_range.front().TokenEnum()));
Expand Down Expand Up @@ -199,6 +227,8 @@ static bool IgnoreWithinStructUnionMemberPartitionGroup(
return true;
}

if (PartitionHasLeadingTokensBeforeForcedWrap(partition)) return true;

// ignore nested structs/unions
if (verible::FindFirstSubtree(
partition.Value().Origin(), [](const Symbol &symbol) {
Expand Down
26 changes: 26 additions & 0 deletions verible/verilog/formatting/formatter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19216,6 +19216,32 @@ TEST(FormatterEndToEndTest,
}
}

// Regression for https://github.com/chipsalliance/verible/issues/2539:
// A // comment followed by a line-continuation `\` before aligned ports must
// not abort in align.h, and must keep the comment on its own line.
TEST(FormatterEndToEndTest, PortListCommentWithLineContinuationDoesNotAbort) {
static constexpr std::string_view kInput =
"module m (\n"
"//\\\n"
"input a\n"
",input b\n"
");\n"
"endmodule\n";
FormatStyle style;
std::ostringstream stream;
const auto status = FormatVerilog(kInput, "<filename>", style, stream);
EXPECT_OK(status) << status.message();
const std::string out = stream.str();
// Must not glue the line-continuation onto the following port declaration.
EXPECT_THAT(out, testing::Not(testing::HasSubstr("//\\ input")));
EXPECT_THAT(out, testing::HasSubstr("//\\"));
EXPECT_THAT(out, testing::HasSubstr("input a"));
EXPECT_THAT(out, testing::HasSubstr("input b"));
EXPECT_THAT(out, testing::HasSubstr("endmodule"));
// Comment line and first port remain on separate lines.
EXPECT_THAT(out, testing::ContainsRegex(R"(//\\\n\s*input a)"));
}

} // namespace
} // namespace formatter
} // namespace verilog
Loading