-
Notifications
You must be signed in to change notification settings - Fork 264
Fix: Compiler directives being merged onto single line (#2073) #2480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes chipsalliance#2073 The formatter was incorrectly merging consecutive compiler directives (like `timescale and `default_nettype) onto a single line instead of keeping them on separate lines. Root cause: IsPreprocessorKeyword() only recognized preprocessor control flow directives (PP_* tokens) but not compiler directives (DR_* tokens), and the tree unwrapper wasn't creating separate partitions for these directives. Changes: - Extended IsPreprocessorKeyword() to include all DR_* compiler directive tokens (timescale, default_nettype, resetall, celldefine, etc.) - Added kTimescaleDirective and kTopLevelDirective nodes to unindented unwrapped line handling in tree unwrapper - Added leaf-level handling for bare DR_* tokens to ensure they start new partitions when at top level - Added comprehensive regression tests for multiple directive combinations All tests pass. Directives now correctly remain on separate lines when at top level, while respecting indentation when inside module ports or other nested contexts.
|
@hzeller : Please share your thoughts on this PR. I believe this fixes the bug. |
hzeller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I suggest to break out the classification of compiler directives into different functions so that it is easier to compose in places where needed.
| case verilog_tokentype::PP_elsif: | ||
| case verilog_tokentype::PP_endif: | ||
| case verilog_tokentype::PP_undef: | ||
| // Compiler directives (DR_*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably make a IsCompilerDirective() classification function, in which we put all these tokens then, so that it has a nice name.
Note, IsPreprocessorKeyword() is also used in formatting/align.cc and formatting/token-annotator.cc - it would probably be good to check if the additional tokens in this IsPreprocessorKeyword classification would be a good addition of what these are doing (add unit tests there).
Then, if having the IsCompilerDirective tokens showed as a win in the updated unit tests, we can
IsPreprocessorKeyword(...) {
...
switch (token_type) {
...
default:
return IsCompilerDirective(token_type);
}
}If not, users of this function can be manually specific if they need both, e.g. by saying IsPreprocessorKeyword(foo) || IsCompilerDirective(foo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hzeller.
I've made the changes recommended. Please let me know if you have any other feedback or areas of improvement.
Created new functions to capture the different categories of directives.
|
Hi @hzeller, Please let me know if there is any additional feedback I could follow. Thanks. |
hzeller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fix: Compiler directives being merged onto single line (#2073)
Problem
The formatter was incorrectly merging consecutive compiler directives onto a single line. For example:
Would be formatted as:
This affected all SystemVerilog compiler directives including
`timescale,`default_nettype,`resetall,`celldefine, and others.Root Cause
The issue had two parts:
Token Classification: The
IsPreprocessorKeyword()function only recognized preprocessor control flow directives (PP_*tokens like`ifdef,`define) but did not include compiler directives (DR_*tokens like`timescale,`default_nettype).Tree Unwrapping: The tree unwrapper was not properly creating separate partitions for:
kTimescaleDirective,kTopLevelDirective)`resetall,`celldefine) that don't have parent nodesChanges
1.
verible/verilog/parser/verilog-token-classifications.ccExtended
IsPreprocessorKeyword()to include allDR_*compiler directive tokens:DR_timescale,DR_default_nettype,DR_resetallDR_celldefine,DR_endcelldefineDR_unconnected_drive,DR_nounconnected_driveDR_suppress_faults,DR_nosuppress_faultsDR_enable_portfaults,DR_disable_portfaultsDR_delay_mode_*,DR_default_decay_time,DR_default_trireg_strengthDR_pragma,DR_uselib,DR_begin_keywords,DR_end_keywordsDR_protect,DR_endprotectThis ensures these tokens receive
kMustWrapspacing decisions in the token annotator.2.
verible/verilog/formatting/tree-unwrapper.cc(Node handling)Added
NodeEnum::kTimescaleDirectiveandNodeEnum::kTopLevelDirectiveto the list of constructs that should be formatted as unindented unwrapped lines, alongside preprocessor control flow clauses.3.
verible/verilog/formatting/tree-unwrapper.cc(Leaf handling)Added handling in the
Visit(SyntaxTreeLeaf)function for preprocessor keyword leaves (bareDR_*tokens without parent nodes). These directives now:4.
verible/verilog/formatting/formatter_test.ccAdded comprehensive regression tests covering:
`timescale+`default_nettype)`resetall+`celldefine+`timescale)`suppress_faults,`enable_portfaults,`delay_mode_*)`default_decay_time,`begin_keywords)All test cases verify that consecutive directives remain on separate lines with no indentation when at top level.
Testing