Skip to content
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

Is there a way to manually keep line wrap #1349

Open
pinkcatfly opened this issue Jun 24, 2022 · 6 comments
Open

Is there a way to manually keep line wrap #1349

pinkcatfly opened this issue Jun 24, 2022 · 6 comments
Labels
formatter Verilog code formatter issues

Comments

@pinkcatfly
Copy link

Test case

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) | 
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) | 
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);

If I want to keep the line wrap here the only way I find is changing '--column_limit'

Actual output

With '--column_limit 80' (just a small limit

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) | 
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) | 
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);

The declaration here is not aligned

With '--column_limit 200' (a large limit

    logic [`just_a_very_long_define_here-1:0]                                    test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) | (the_input_used_to_illustrate_1 ? result_1 : 'b0) | (the_input_used_to_illustrate_2 ? result_2 : 'b0);

The declaration here is aligned BUT the assign is ruined....

I wonder is there is a way to keep the line wrap as it is without using directive 'format off'

Expected or suggested output

    logic [`just_a_very_long_define_here-1:0]                                    test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) | 
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) | 
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);
@pinkcatfly pinkcatfly added the formatter Verilog code formatter issues label Jun 24, 2022
@hzeller
Copy link
Collaborator

hzeller commented Jun 24, 2022

There are two ways that you can make the current version of Verible do this.

Either by suppling a comment telling it to skip formatting for some lines

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    // verilog_format: off
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) | 
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) | 
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);
    // verilog_format: on

Or, by adding EOL-comments at the end of the lines you don't want to have wrapped. Sometimes that is also useful to encourage documentation:

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) |   // one thing
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) |   // another thing
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);    // the third thing.

Having said that, maybe we could find some heuristics ('looks like similar expressions are aligned in similar ways already, maybe this is what we should also be doing') that preserves these. But then again, more heuristics often mean more surprising results. CC @mglb if he has some ideas.

@pinkcatfly
Copy link
Author

Thanks for reply. But another issue comes up. I dont want to put too many comments in the code so the second way I tried. Format the code I get this

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) |  // one thing
        (the_input_used_to_illustrate_1 ? result_1 : 'b0) |  // another thing
        (the_input_used_to_illustrate_2 ? result_2 : 'b0);  // the third thing.

But what I want is

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) |  //pattern one thing
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) |  // another thing
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);  // the third thing.

So the second solution is not perfects. BUT under certain circumstance verible will leave the line untouched.However I havent found any pattern so far.

@hzeller
Copy link
Collaborator

hzeller commented Jun 27, 2022

CC @mglb (who did a lot of formatting work) I think it might be a good idea to have some alignment of expressions, maybe if they are parenthesized. So if we have parenthesis around the whole expression, making sure that any sub-expression never is indented less than the opening position (or position+1) of the starting parenthesis in subsequent lines:

assign out = ((the_input_used_to_illustrate_0 ? result_0 : 'b0) |  //pattern one thing
              (the_input_used_to_illustrate_1 ? result_1 : 'b0) |  // another thing
              (the_input_used_to_illustrate_2 ? result_2 : 'b0));  // the third thing.

@pinkcatfly
Copy link
Author

CC @mglb (who did a lot of formatting work) I think it might be a good idea to have some alignment of expressions, maybe if they are parenthesized. So if we have parenthesis around the whole expression, making sure that any sub-expression never is indented less than the opening position (or position+1) of the starting parenthesis in subsequent lines:

assign out = ((the_input_used_to_illustrate_0 ? result_0 : 'b0) |  //pattern one thing
              (the_input_used_to_illustrate_1 ? result_1 : 'b0) |  // another thing
              (the_input_used_to_illustrate_2 ? result_2 : 'b0));  // the third thing.

An excellent idea. Maybe I can be one developer of this feature when I finish what I'm doing now. It is TOO hard to find a suitable formatter for verilog and SV....

@hzeller
Copy link
Collaborator

hzeller commented Jun 27, 2022

@mglb @tgorochowik (and maybe @fangism ): can you help @pinkcatfly find the right place to get started ?

@mglb
Copy link
Contributor

mglb commented Jun 27, 2022

Thanks for reply. But another issue comes up. I dont want to put too many comments in the code so the second way I tried. Format the code I get this

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) |  // one thing
        (the_input_used_to_illustrate_1 ? result_1 : 'b0) |  // another thing
        (the_input_used_to_illustrate_2 ? result_2 : 'b0);  // the third thing.

But what I want is

    logic [`just_a_very_long_define_here-1:0] test_varible_0;
    logic [`just_a_very_long_define_here-1:0][`just_a_very_long_define_here-1:0] test_varible_0;
    assign out = (the_input_used_to_illustrate_0 ? result_0 : 'b0) |  //pattern one thing
                 (the_input_used_to_illustrate_1 ? result_1 : 'b0) |  // another thing
                 (the_input_used_to_illustrate_2 ? result_2 : 'b0);  // the third thing.

So the second solution is not perfects. BUT under certain circumstance verible will leave the line untouched.However I havent found any pattern so far.

I did some initial work to make things like this work, but I don't have time to continue this currently.
The way I wanted to go with this (roughly):

  • Refactor TokenPartitionTree to allow use of Choice operator from LayoutOptimizer in TreeUnwrapper. I've stared implementing this, code can be found here: [WIP] TokenPartitionTree refactoring / "Choice" nodes support #1328. This is completed in more than 50%.

  • Specify layouts for assignments in TreeUnwrapper with combinations of Juxtaposition, Stack, and Wrap layouts. I did proof of concept for assignment of a string initializer here (note that this can be hard to build): https://github.com/antmicro/verible/blob/c0997f7beb0819232bfe6015eae2f0b6da05acaa/verilog/formatting/tree_unwrapper.cc#L2667-L2749 (most important is the second half of the selected code). This code defines 3 possible layouts for assignment of string initializer which look like these:

    string abc[] = {"a", "b", "c"};
    
    string abc[] = {"a",
                    "b",
                    "c"};
    
    string abc[] = {
      "a",
      "b",
      "c"
    };
    

    In your case you would need to put nodes past = in a Wrap layout, then put assign out = and the wrap layout in Juxtaposition layout. Possibly you would also need to play with the way how nodes (uwlines) past = are organized.

You can also look at current implementation of string initializer assignment. It uses a bit less flexible alignment policy (which might be hard to adapt to your case), but the result is the same as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Verilog code formatter issues
Projects
None yet
Development

No branches or pull requests

3 participants