Skip to content

Commit

Permalink
Align case items (statements).
Browse files Browse the repository at this point in the history 
Initially, this only does two-column alignment (after the colon), nothing smarter.

e.g:
  case (x)
    kFooo: xx = y;
    kBar:  y = xx;
  endcase

This also fixes handling of //comments in case item lists.

Next: related cases, like case-inside, randcase, generate case...

issues #413

PiperOrigin-RevId: 331254916
  • Loading branch information
@fangism @hzeller
fangism authored and hzeller committed Sep 12, 2020
1 parent 4e5e98e commit b805ed8
Show file tree
Hide file tree
Showing 7 changed files with 258 additions and 5 deletions.
7 changes: 5 additions & 2 deletions common/formatting/align.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,11 @@ static void CommitAlignmentDecisionToRow(
decision = SpacingOptions::MustAppend;
}
}
partition.Value().SetPartitionPolicy(
PartitionPolicyEnum::kSuccessfullyAligned);
// Tag every subtree as having already been committed to alignment.
partition.ApplyPostOrder([](TokenPartitionTree& node) {
node.Value().SetPartitionPolicy(
PartitionPolicyEnum::kSuccessfullyAligned);
});
}
}

Expand Down
97 changes: 97 additions & 0 deletions verilog/formatting/align.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,30 @@ static bool IgnoreWithinActualNamedPortPartitionGroup(
return false;
}

static bool TokenForcesLineBreak(const PreFormatToken& ftoken) {
switch (ftoken.TokenEnum()) {
case verilog_tokentype::TK_begin:
case verilog_tokentype::TK_fork:
return true;
}
return false;
}

static bool IgnoreMultilineCaseStatements(const TokenPartitionTree& partition) {
if (IgnoreCommentsAndPreprocessingDirectives(partition)) return true;

const auto& uwline = partition.Value();
const auto token_range = uwline.TokensRange();

// Scan for any tokens that would force a line break.
if (std::any_of(token_range.begin(), token_range.end(),
&TokenForcesLineBreak)) {
return true;
}

return false;
}

// This class marks up token-subranges in named parameter assignments for
// alignment. e.g. ".parameter_name(value_expression)"
class ActualNamedParameterColumnSchemaScanner : public ColumnSchemaScanner {
Expand Down Expand Up @@ -689,6 +713,66 @@ class ParameterDeclarationColumnSchemaScanner : public ColumnSchemaScanner {
bool new_column_after_open_bracket_ = false;
};

// This class marks up token-subranges in case items for alignment.
// e.g. "value1, value2: x = f(y);"
class CaseItemColumnSchemaScanner : public ColumnSchemaScanner {
public:
CaseItemColumnSchemaScanner() = default;

bool ParentContextIsCaseItem() const {
return Context().DirectParentIsOneOf(
{NodeEnum::kCaseItem, NodeEnum::kDefaultItem});
}

void Visit(const SyntaxTreeNode& node) override {
auto tag = NodeEnum(node.Tag().tag);
VLOG(2) << __FUNCTION__ << ", node: " << tag << " at "
<< TreePathFormatter(Path());

if (previous_token_was_case_colon_) {
if (ParentContextIsCaseItem()) {
ReserveNewColumn(node, FlushLeft);
previous_token_was_case_colon_ = false;
}
} else {
switch (tag) {
case NodeEnum::kCaseItem:
case NodeEnum::kDefaultItem: {
// Start a new column right away.
ReserveNewColumn(node, FlushLeft);
break;
}
default:
break;
}
}

// recursive visitation
TreeContextPathVisitor::Visit(node);
VLOG(2) << __FUNCTION__ << ", leaving node: " << tag;
}

void Visit(const SyntaxTreeLeaf& leaf) override {
VLOG(2) << __FUNCTION__ << ", leaf: " << leaf.get() << " at "
<< TreePathFormatter(Path());
const int tag = leaf.get().token_enum();
switch (tag) {
case ':':
if (ParentContextIsCaseItem()) {
// mark the next node as the start of a new column
previous_token_was_case_colon_ = true;
}
break;
default:
break;
}
VLOG(2) << __FUNCTION__ << ", leaving leaf: " << leaf.get();
}

private:
bool previous_token_was_case_colon_ = false;
};

static const verible::AlignedFormattingHandler kPortDeclarationAligner{
.extract_alignment_groups = verible::ExtractAlignmentGroupsAdapter(
&verible::GetSubpartitionsBetweenBlankLines,
Expand Down Expand Up @@ -729,6 +813,14 @@ static const verible::AlignedFormattingHandler kClassPropertyAligner{
AlignmentCellScannerGenerator<ClassPropertyColumnSchemaScanner>(),
};

static const verible::AlignedFormattingHandler kCaseItemAligner{
.extract_alignment_groups = verible::ExtractAlignmentGroupsAdapter(
&verible::GetSubpartitionsBetweenBlankLines,
&IgnoreMultilineCaseStatements),
.alignment_cell_scanner =
AlignmentCellScannerGenerator<CaseItemColumnSchemaScanner>(),
};

struct AlignedFormattingConfiguration {
// Set of functions for driving specific code aligners.
verible::AlignedFormattingHandler handler;
Expand Down Expand Up @@ -794,6 +886,11 @@ void TabularAlignTokenPartitions(TokenPartitionTree* partition_ptr,
[](const FormatStyle& vstyle) {
return vstyle.class_member_variable_alignment;
}}},
{NodeEnum::kCaseItemList,
{kCaseItemAligner,
[](const FormatStyle& vstyle) {
return vstyle.case_items_alignment;
}}},
};
const auto handler_iter = kAlignHandlers->find(NodeEnum(node->Tag().tag));
if (handler_iter == kAlignHandlers->end()) return;
Expand Down
5 changes: 5 additions & 0 deletions verilog/formatting/format_style.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ struct FormatStyle : public verible::BasicFormatStyle {
// Internal tests assume these are forced to kAlign.
AlignmentPolicy class_member_variable_alignment = AlignmentPolicy::kAlign;

// Control how case items are formatted.
// Internal tests assume these are forced to kAlign.
AlignmentPolicy case_items_alignment = AlignmentPolicy::kAlign;

// At this time line wrap optimization is problematic and risks ruining
// otherwise reasonable code. When set to false, this switch will make the
// formatter give-up and leave code as-is in cases where it would otherwise
Expand Down Expand Up @@ -117,6 +121,7 @@ struct FormatStyle : public verible::BasicFormatStyle {
module_net_variable_alignment = policy;
formal_parameters_alignment = policy;
class_member_variable_alignment = policy;
case_items_alignment = policy;
}
};

Expand Down
117 changes: 115 additions & 2 deletions verilog/formatting/formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3836,6 +3836,19 @@ static constexpr FormatterTestCase kFormatterTestCases[] = {
" end\n"
" endcase\n"
"endfunction\n"},
{// case statement, interleaved with comments
"function f; case (x) \n//c1\nState0 : a=b;//c2\n//c3\n State1 : "
"a=b;//c4\n//c5\n "
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" //c1\n"
" State0: a = b; //c2\n"
" //c3\n"
" State1: a = b; //c4\n"
" //c5\n"
" endcase\n"
"endfunction\n"},
{// case inside statement
"function f; case (x)inside k1 : return b; k2 : begin return b; end "
"endcase endfunction\n",
Expand Down Expand Up @@ -3884,7 +3897,7 @@ static constexpr FormatterTestCase kFormatterTestCases[] = {
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" k1: if (b) break;\n"
" k1: if (b) break;\n" // aligned
" default: return 2;\n"
" endcase\n"
"endfunction\n"},
Expand All @@ -3893,7 +3906,7 @@ static constexpr FormatterTestCase kFormatterTestCases[] = {
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" k1: break;\n"
" k1: break;\n" // aligned
" default: if (c) return 2;\n"
" endcase\n"
"endfunction\n"},
Expand Down Expand Up @@ -7152,6 +7165,106 @@ TEST(FormatterEndToEndTest, AutoInferAlignment) {
" int my_int;\n" // ... but still indent
" foo_pkg::bar_t my_bar;\n"
"endclass : cc\n"},

// case item test cases
{// small difference between flush-left and align, so align
"function f; case (x)kZZZZ :if( b )break; default :return 2;"
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" kZZZZ: if (b) break;\n" // aligned, only adds 2 spaces
" default: return 2;\n"
" endcase\n"
"endfunction\n"},
{// small error relative to flush-left, so flush-left
"function f; case (x)kZ :if( b )break; default :return 2;"
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" kZ: if (b) break;\n" // flush-left
" default: return 2;\n"
" endcase\n"
"endfunction\n"},
{// intentional spacing error (delta=4) induces alignment
"function f; case (x)kZ :if( b )break; default :return 2;"
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" kZ: if (b) break;\n"
" default: return 2;\n"
" endcase\n"
"endfunction\n"},
{// induced alignment, with ignored comments
"function f; case (x)kZ :if( b )break; \n//c1\n kXX: g = f; "
"\n//c2\ndefault :return 2;"
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" kZ: if (b) break;\n"
" //c1\n"
" kXX: g = f;\n"
" //c2\n"
" default: return 2;\n"
" endcase\n"
"endfunction\n"},
{// induced alignment, ignore multiline case item in the middle
"function f; case (x)"
"kZ :if( b )break; "
"kYY :return 2;" // excess spaces induce alignment
" kXXXXXXXXX: begin end" // multi-line, ignored
" kWWWWW: cc = 23;\n"
" kVVV: cd = 24;\n"
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" kZ: if (b) break;\n"
" kYY: return 2;\n"
" kXXXXXXXXX: begin\n" // separates above/below groups
" end\n"
" kWWWWW: cc = 23;\n"
" kVVV: cd = 24;\n" // aligned
" endcase\n"
"endfunction\n"},
{// induced alignment, ignore multiline case item in the middle
"function f; case (x)"
"kZ :if( b )break; "
"kYY :return 2;" // excess spaces induce alignment
" kXXXXXXXXX: if(w)begin end" // multi-line, ignored
" kWWWWW: cc = 23;\n"
" kVVV: cd = 24;\n"
"endcase endfunction\n",
"function f;\n"
" case (x)\n"
" kZ: if (b) break;\n"
" kYY: return 2;\n"
" kXXXXXXXXX:\n" // TODO(fangism): allow this to merge with if()
// else indent the following two lines
" if (w) begin\n" // separates above/below groups
" end\n"
" kWWWWW: cc = 23;\n"
" kVVV: cd = 24;\n" // aligned
" endcase\n"
"endfunction\n"},
{// induced alignment, ignore multiline case item in the middle
"task t; case (x)"
"kZ :if( b )break; "
"kYY :return 2;" // excess spaces induce alignment
" kXXXXXXXXX: fork join" // multi-line, ignored
" kWWWWW: cc = 23;\n"
" kVVV: cd = 24;\n"
"endcase endtask\n",
"task t;\n"
" case (x)\n"
" kZ: if (b) break;\n"
" kYY: return 2;\n"
" kXXXXXXXXX:\n" // TODO(fangism): allow this to merge with fork
// else indent the following two lines
" fork\n" // separates above/below groups
" join\n"
" kWWWWW: cc = 23;\n"
" kVVV: cd = 24;\n" // aligned
" endcase\n"
"endtask\n"},
};
// Use a fixed style.
FormatStyle style;
Expand Down
3 changes: 2 additions & 1 deletion verilog/formatting/tree_unwrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ void TreeUnwrapper::InterChildNodeHook(const SyntaxTreeNode& node) {
case NodeEnum::kPackageItemList:
case NodeEnum::kSpecifyItemList:
case NodeEnum::kBlockItemStatementList:
case NodeEnum::kCaseItemList:
case NodeEnum::kConstraintExpressionList:
case NodeEnum::kConstraintBlockItemList:
LookAheadBeyondCurrentNode();
Expand Down Expand Up @@ -944,7 +945,6 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
case NodeEnum::kPackageItemList:
case NodeEnum::kInterfaceClassDeclaration:
case NodeEnum::kGenerateItemList:
case NodeEnum::kCaseItemList:
case NodeEnum::kCaseInsideItemList:
case NodeEnum::kCasePatternItemList:
case NodeEnum::kGenerateCaseItemList:
Expand Down Expand Up @@ -1004,6 +1004,7 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
}
}

case NodeEnum::kCaseItemList:
case NodeEnum::kClassItems:
case NodeEnum::kModuleItemList: {
const int indent = suppress_indentation ? 0 : style_.indentation_spaces;
Expand Down
29 changes: 29 additions & 0 deletions verilog/formatting/tree_unwrapper_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6890,6 +6890,35 @@ const TreeUnwrapperTestData kUnwrapFunctionTestCases[] = {
L(0, {"endfunction"})),
},

{
"function with case statement, with comments",
"function foo_case;"
"case (y) \n"
"//c1\n"
"k1: return 0;\n"
"//c2\n"
"k2: return 1;\n"
"//c3\n"
"endcase "
"endfunction",
FunctionDeclaration(
0, FunctionHeader(0, {"function", "foo_case", ";"}),
FlowControl(1, L(1, {"case", "(", "y", ")"}),
CaseItemList(2, //
L(2, {"//c1"}), //
N(2, //
L(2, {"k1", ":"}), //
L(2, {"return", "0", ";"})),
L(2, {"//c2"}), //
N(2, //
L(2, {"k2", ":"}), //
L(2, {"return", "1", ";"})),
L(2, {"//c3"}) //
), //
L(1, {"endcase"})), //
L(0, {"endfunction"})),
},

{
"function with case statements",
"function foo_case;"
Expand Down
5 changes: 5 additions & 0 deletions verilog/tools/formatter/verilog_format.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ ABSL_FLAG(AlignmentPolicy, formal_parameters_alignment,
ABSL_FLAG(AlignmentPolicy, class_member_variables_alignment,
AlignmentPolicy::kInferUserIntent,
"Format class member variables: {align,flush-left,preserve,infer}");
ABSL_FLAG(AlignmentPolicy, case_items_alignment,
AlignmentPolicy::kInferUserIntent,
"Format case items: {align,flush-left,preserve,infer}");

ABSL_FLAG(bool, try_wrap_long_lines, false,
"If true, let the formatter attempt to optimize line wrapping "
Expand Down Expand Up @@ -236,6 +239,8 @@ static bool formatOneFile(absl::string_view filename,
absl::GetFlag(FLAGS_formal_parameters_alignment);
format_style.class_member_variable_alignment =
absl::GetFlag(FLAGS_class_member_variables_alignment);
format_style.case_items_alignment =
absl::GetFlag(FLAGS_case_items_alignment);
}

std::ostringstream stream;
Expand Down

0 comments on commit b805ed8

Please sign in to comment.