Skip to content

Commit

Permalink
Fix alignment when first column (direction) is missing.
Browse files Browse the repository at this point in the history 
Test case now produces:
  module somefunction (
      input logic clk,
      input int   a,
            int   b
  );
  endmodule

Even if this code is not style-compliant, the alignment helps visualize the
empty cells, and can alert a user to what's missing.

issues #28

PiperOrigin-RevId: 314631482
  • Loading branch information
@fangism @hzeller
fangism authored and hzeller committed Jun 4, 2020
1 parent e48485e commit 3c535e1
Show file tree
Hide file tree
Showing 14 changed files with 373 additions and 116 deletions.
68 changes: 53 additions & 15 deletions common/formatting/align.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ static int GetPartitionNodeEnum(const TokenPartitionTree& partition) {

static bool VerifyRowsOriginalNodeTypes(
const std::vector<TokenPartitionIterator>& rows) {
VLOG(1) << __FUNCTION__;
const auto first_node_type = GetPartitionNodeEnum(*rows.front());
for (const auto& row : verible::make_range(rows.begin() + 1, rows.end())) {
const auto node_type = GetPartitionNodeEnum(*row);
Expand All @@ -112,16 +113,16 @@ static int EffectiveCellWidth(const FormatTokenRange& tokens) {
VLOG(2) << __FUNCTION__;
// Sum token text lengths plus required pre-spacings (except first token).
// Note: LeadingSpacesLength() honors where original spacing when preserved.
return std::accumulate(tokens.begin(), tokens.end(),
-tokens.front().LeadingSpacesLength(),
[](int total_width, const PreFormatToken& ftoken) {
VLOG(2) << " +" << ftoken.before.spaces_required
<< " +" << ftoken.token->text().length();
// TODO(fangism): account for multi-line tokens like
// block comments.
return total_width + ftoken.LeadingSpacesLength() +
ftoken.token->text().length();
});
return std::accumulate(
tokens.begin(), tokens.end(), -tokens.front().LeadingSpacesLength(),
[](int total_width, const PreFormatToken& ftoken) {
const int pre_width = ftoken.LeadingSpacesLength();
const int text_length = ftoken.token->text().length();
VLOG(2) << " +" << pre_width << " +" << text_length;
// TODO(fangism): account for multi-line tokens like
// block comments.
return total_width + ftoken.LeadingSpacesLength() + text_length;
});
}

static int EffectiveLeftBorderWidth(const MutableFormatTokenRange& tokens) {
Expand Down Expand Up @@ -355,6 +356,10 @@ static void ComputeCellWidths(AlignmentMatrix* matrix) {
for (auto& cell : row) {
cell.UpdateWidths();
}
// Force leftmost table border to be 0 because these cells start new lines
// and thus should not factor into alignment calculation.
// Note: this is different from how StateNode calculates column positions.
row.front().left_border_width = 0;
}
VLOG(2) << "end of " << __FUNCTION__ << ", cell sizes:\n"
<< MatrixCellSizeFormatter{*matrix};
Expand Down Expand Up @@ -396,7 +401,10 @@ static void AlignRowSpacings(
// Align by setting the left-spacing based on sum of cell widths
// before this one.
const int padding = column_iter->width - cell.compact_width;
int& left_spacing = cell.tokens.front().before.spaces_required;
PreFormatToken& ftoken = cell.tokens.front();
ftoken.before.break_decision =
SpacingOptions::AppendAligned; // force printing of spaces
int& left_spacing = ftoken.before.spaces_required;
if (properties_iter->flush_left) {
left_spacing = accrued_spaces;
accrued_spaces = padding;
Expand Down Expand Up @@ -434,6 +442,15 @@ static MutableFormatTokenRange ConvertToMutableFormatTokenRange(
ConvertToMutableIterator<array_type>(const_range.end(), base));
}

static FormatTokenRange EpilogRange(const TokenPartitionTree& partition,
const AlignmentRow& row) {
// Identify the unaligned epilog tokens of this 'partition', i.e. those not
// spanned by 'row'.
auto partition_end = partition.Value().TokensRange().end();
auto row_end = row.back().tokens.end();
return FormatTokenRange(row_end, partition_end);
}

static MutableFormatTokenRange GetMutableFormatTokenRange(
const UnwrappedLine& unwrapped_line,
MutableFormatTokenRange::iterator ftoken_base) {
Expand Down Expand Up @@ -544,10 +561,7 @@ static void AlignFilteredRows(
for (const auto& row : matrix) {
if (!row.empty()) {
// Identify the unaligned epilog text on each partition.
auto partition_end =
partition_iter->base()->Value().TokensRange().end();
auto row_end = row.back().tokens.end();
const FormatTokenRange epilog_range(row_end, partition_end);
const FormatTokenRange epilog_range(EpilogRange(**partition_iter, row));
const int aligned_partition_width =
total_column_width + EffectiveCellWidth(epilog_range);
if (aligned_partition_width > column_limit) {
Expand All @@ -565,10 +579,34 @@ static void AlignFilteredRows(
// exceed the column limit, then for now, refuse to align.
// TODO(fangism): implement overflow mitigation fallback strategies.

// At this point, the proposed alignment/padding 'fits'.

// Adjust pre-token spacings of each row to align to the column configs.
for (auto& row : matrix) {
AlignRowSpacings(column_configs, column_properties, &row);
}

// Signal that these partitions spacing/wrapping decisions have already been
// solved (append everything because they fit on one line).
{
auto partition_iter = rows.begin();
for (auto& row : matrix) {
if (!row.empty()) {
TokenPartitionTree& partition(**partition_iter);
const auto ftoken_range = ConvertToMutableFormatTokenRange(
partition.Value().TokensRange(), ftoken_base);
for (auto& ftoken : ftoken_range) {
SpacingOptions& decision = ftoken.before.break_decision;
if (decision == SpacingOptions::Undecided) {
decision = SpacingOptions::MustAppend;
}
}
partition.Value().SetPartitionPolicy(
PartitionPolicyEnum::kSuccessfullyAligned);
}
++partition_iter;
}
}
VLOG(1) << "end of " << __FUNCTION__;
}

Expand Down
125 changes: 81 additions & 44 deletions common/formatting/align_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,19 +100,6 @@ TEST_F(TabularAlignTokenTest, EmptyPartitionRange) {
// so they end up pointing to the same ranges anyway.
}

// Kludge: The constructor for FormattedExcerpt overwrites the before.spaces of
// the first token using the UnwrappedLine's indentation_spaces, which discards
// any space padding done to the left of the first token.
// Here, we explicitly combine the original indentation with the padded spacing
// to get the desired result.
static FormattedExcerpt CreateFormattedExcerpt(const UnwrappedLine& uwline) {
UnwrappedLine copy(uwline);
copy.SetIndentationSpaces(
uwline.IndentationSpaces() +
uwline.TokensRange().front().before.spaces_required);
return FormattedExcerpt(copy);
}

class Sparse3x3MatrixAlignmentTest : public AlignmentTestFixture {
public:
Sparse3x3MatrixAlignmentTest()
Expand Down Expand Up @@ -168,7 +155,7 @@ class Sparse3x3MatrixAlignmentTest : public AlignmentTestFixture {
std::string Render() const {
std::ostringstream stream;
for (const auto& child : partition_.Children()) {
stream << CreateFormattedExcerpt(child.Value()) << std::endl;
stream << FormattedExcerpt(child.Value()) << std::endl;
}
return stream.str();
}
Expand Down Expand Up @@ -201,6 +188,7 @@ TEST_F(Sparse3x3MatrixAlignmentTest, ZeroInterTokenPadding) {

TEST_F(Sparse3x3MatrixAlignmentTest, OneInterTokenPadding) {
// Require 1 space between tokens.
// Will have no effect on the first token in each partition.
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
Expand All @@ -212,9 +200,9 @@ TEST_F(Sparse3x3MatrixAlignmentTest, OneInterTokenPadding) {

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n"
" three four\n"
" five six\n");
" one two\n"
"three four\n"
"five six\n");
}

TEST_F(Sparse3x3MatrixAlignmentTest, OneInterTokenPaddingExceptFront) {
Expand All @@ -239,13 +227,10 @@ TEST_F(Sparse3x3MatrixAlignmentTest, OneInterTokenPaddingExceptFront) {
}

TEST_F(Sparse3x3MatrixAlignmentTest, RightFlushed) {
// Require 1 space between tokens, except ones at the beginning of partitions.
// Require 1 space between tokens.
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
pre_format_tokens_[0].before.spaces_required = 0;
pre_format_tokens_[2].before.spaces_required = 0;
pre_format_tokens_[4].before.spaces_required = 0;

TabularAlignTokens(
&partition_, AlignmentCellScannerGenerator<TokenColumnizerRightFlushed>(),
Expand All @@ -264,9 +249,7 @@ TEST_F(Sparse3x3MatrixAlignmentTest, OneInterTokenPaddingWithIndent) {
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
pre_format_tokens_[0].before.spaces_required = 0;
pre_format_tokens_[2].before.spaces_required = 0;
pre_format_tokens_[4].before.spaces_required = 0;
// Indent each partition.
for (auto& child : partition_.Children()) {
child.Value().SetIndentationSpaces(4);
}
Expand All @@ -288,6 +271,9 @@ TEST_F(Sparse3x3MatrixAlignmentTest, IgnoreCommentLine) {
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
// Leave the 'commented' line indented.
pre_format_tokens_[2].before.break_decision = SpacingOptions::MustWrap;
partition_.Children()[1].Value().SetIndentationSpaces(1);

// Pretend lines that begin with "three" are to be ignored, like comments.
auto ignore_threes = [](const TokenPartitionTree& partition) {
Expand All @@ -299,10 +285,10 @@ TEST_F(Sparse3x3MatrixAlignmentTest, IgnoreCommentLine) {
ignore_threes, pre_format_tokens_.begin(), sample_, ByteOffsetSet(), 40);

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n" // is aligned
" three four\n" // this line does not participate in alignment
" five six\n" // is aligned
EXPECT_EQ(Render(), //
" one two\n" // is aligned
" three four\n" // this line does not participate in alignment
"five six\n" // is aligned
);
}

Expand All @@ -321,9 +307,35 @@ TEST_F(Sparse3x3MatrixAlignmentTest, CompletelyDisabledNoAlignment) {

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n"
" three four\n"
" five six\n");
"one two\n"
"three four\n"
"five six\n");
}

TEST_F(Sparse3x3MatrixAlignmentTest, CompletelyDisabledNoAlignmentWithIndent) {
// Require 1 space between tokens.
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
for (auto& child : partition_.Children()) {
child.Value().SetIndentationSpaces(3);
}
pre_format_tokens_[0].before.break_decision = SpacingOptions::MustWrap;
pre_format_tokens_[2].before.break_decision = SpacingOptions::MustWrap;
pre_format_tokens_[4].before.break_decision = SpacingOptions::MustWrap;

TabularAlignTokens(
&partition_, AlignmentCellScannerGenerator<TokenColumnizer>(),
[](const TokenPartitionTree&) { return false; },
pre_format_tokens_.begin(), sample_,
// Alignment disabled over entire range.
ByteOffsetSet({{0, static_cast<int>(sample_.length())}}), 40);

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n"
" three four\n"
" five six\n");
}

TEST_F(Sparse3x3MatrixAlignmentTest, PartiallyDisabledNoAlignment) {
Expand All @@ -342,9 +354,9 @@ TEST_F(Sparse3x3MatrixAlignmentTest, PartiallyDisabledNoAlignment) {

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n"
" three four\n"
" five six\n");
"one two\n"
"three four\n"
"five six\n");
}

TEST_F(Sparse3x3MatrixAlignmentTest, DisabledByColumnLimit) {
Expand All @@ -357,14 +369,39 @@ TEST_F(Sparse3x3MatrixAlignmentTest, DisabledByColumnLimit) {
&partition_, AlignmentCellScannerGenerator<TokenColumnizer>(),
[](const TokenPartitionTree&) { return false; },
pre_format_tokens_.begin(), sample_, ByteOffsetSet(),
// Column limit chosen to be smaller than sum of columns' widths (6+4+5):
14);
// Column limit chosen to be smaller than sum of columns' widths.
// 5 (no left padding) +4 +5 = 14, so we choose 13
13);

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n"
" three four\n"
" five six\n");
"one two\n"
"three four\n"
"five six\n");
}

TEST_F(Sparse3x3MatrixAlignmentTest, DisabledByColumnLimitIndented) {
// Require 1 space between tokens.
for (auto& ftoken : pre_format_tokens_) {
ftoken.before.spaces_required = 1;
}
for (auto& child : partition_.Children()) {
child.Value().SetIndentationSpaces(3);
}

TabularAlignTokens(
&partition_, AlignmentCellScannerGenerator<TokenColumnizer>(),
[](const TokenPartitionTree&) { return false; },
pre_format_tokens_.begin(), sample_, ByteOffsetSet(),
// Column limit chosen to be smaller than sum of columns' widths.
// 3 (indent) +5 (no left padding) +4 +5 = 17, so we choose 16
16);

// Verify string rendering of result.
EXPECT_EQ(Render(), //
"one two\n"
"three four\n"
"five six\n");
}

class MultiAlignmentGroupTest : public AlignmentTestFixture {
Expand Down Expand Up @@ -442,7 +479,7 @@ class MultiAlignmentGroupTest : public AlignmentTestFixture {
const auto newlines =
std::max<int>(std::count(spaces.begin(), spaces.end(), '\n') - 1, 0);
stream << Spacer(newlines, '\n');
stream << CreateFormattedExcerpt(child.Value()) << std::endl;
stream << FormattedExcerpt(child.Value()) << std::endl;
position = tokens_range.back().token->right(text);
}
return stream.str();
Expand All @@ -469,11 +506,11 @@ TEST_F(MultiAlignmentGroupTest, BlankLineSeparatedGroups) {

// Verify string rendering of result.
EXPECT_EQ(Render(), //
" one two\n"
" three four\n"
" one two\n"
"three four\n"
"\n" // preserve blank line
" five seven\n"
" six eight\n");
"five seven\n"
" six eight\n");
}

// TODO(fangism): test case that demonstrates repeated constructs in a deeper
Expand Down
Loading

0 comments on commit 3c535e1

Please sign in to comment.