How to align named ports in module instantiations?

[imphil]

How do we want to format named port lists in module instantiations in SystemVerilog source code? The style guide is currently silent on this topic, but uses in examples the "align" formatting shown below.

Verible give us the following options:

    --named_port_alignment (Format named port connections:
      {align,flush-left,preserve,infer}); default: infer;

align and flush-left are the actual choices. preserve doesn't change formatting, and infer tries to guess the formatting based on what the code used, which isn't a good choice for a style guide.

Option 1: align

Verible with --named_port_alignment=align. Note that tabular alignment is always done in "blocks" (as indicated by an empty line in between).

module my_module;

  mod u_mod (
      .clk_i,
      .rst_ni,
      .in_1 (my_signal_in),
      .out_2(my_signal_out),

      .in_another_block_i(my_signal_in),
      .in3               (something)
  );

endmodule

Option 2: flush-left

Verible with --named_port_alignment=flush-left:

module my_module;

  mod u_mod (
      .clk_i,
      .rst_ni,
      .in_1(my_signal_in),
      .out_2(my_signal_out),

      .in_another_block_i(my_signal_in),
      .in3(something)
  );

endmodule

[sriyerg]

Option 1 preferred.

[msfschaffner]

I also have a slight preference for option 1.

[rswarbrick]

Option 1 for me. Is it possible to add an extra space (so there is a gap after out_2 and in_another_block_i)?

[mdhayter]

Option 1 seems closest to our current style.

Do comments restart the indentation or only completely blank lines? It seems useful to be able to put comments at the top of groups of signals without changing the indentation?

[GregAC]

Personally I like alignment (so Option 1), I find it makes it easier to understand connectivity at a glance. I am not too fussed about the precise details of that alignment.

[imphil]

@mdhayter wrote:

Do comments restart the indentation or only completely blank lines? It seems useful to be able to put comments at the top of groups of signals without changing the indentation?

Only at least one empty line indicates a new block, as the following example shows. This allows comments within a block and is consistent with e.g. Markdown, Latex or other languages which use an empty line as block/paragraph separator.

@rswarbrick wrote:

Is it possible to add an extra space (so there is a gap after out_2 and in_another_block_i)?

I'm not fully sure I understood you correctly there, but if you meant having more than one empty line between groups/blocks of ports: yes, that's possible. I've also included that in the example below.

module my_module;

  mod u_mod (
      .clk_i,
      .rst_ni,
      .in_1 (my_signal_in),
      .out_2(my_signal_out),

      .in_another_block_i(my_signal_in),
      .in3               (something),
      // some comment
      .in24              (my_signal_in),
      .in3               (something),

      // another comment
      .in2224(my_signal_in),
      .in23  (something)
  );

endmodule

@rswarbrick @mdhayter is this the behavior you were hoping for? (I think so?)

[rswarbrick]

Oh, sorry. No I meant an extra space (0x20):

module my_module;

  mod u_mod (
      .clk_i,
      .rst_ni,
      .in_1  (my_signal_in),
      .out_2 (my_signal_out),

      .in_another_block_i (my_signal_in),
      .in3                (something)
  );

endmodule

[imphil]

@rswarbrick Oh I see. (No) spaces before the opening parentheses are another topic which our style guide doesn't explicitly spell out, but it (reasonably consistently) uses the syntax Verible enforces as well (https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#parameterized-module-instantiation). The "space/no space before parentheses" topic is very inconsistent (and I don't think easy to get consistent in SystemVerilog). Maybe you could have a look and file another issue for it?

[rswarbrick]

Ah, cool. I don't really have strong feelings: if it's less technical work, happy to standardise without the space.

[imphil]

Thanks for the discussion! A pull request implementing this guidance is now available at #50. Please have a look and approve if you're happy with it!

[eunchan]

Sorry for late jump in. Just put my though here. But feel free to move on.

Maybe I am an outlier to this. I personally think restricting the port list style is too much. Either styles aren't that bad in terms of readability at least for me. I think at least we can have wiggle room to choose among these (+ more , e.g: Adding space inside parentheses) by individual designers.

[imphil]

@eunchan I can understand the desire to have more formatting freedom (I often enough disagree with style guides!). However, having more than one way of formatting code can lead to tedious discussions in PRs about the "preferred" way (tastes differ!), and prevents automatic formatters from working reliably. I'm therefore trying to standardize on a single way of doing things whenever possible.

We are not enforcing an automated SystemVerilog formatter at the moment (this will be a separate discussion), but the clarifications in the style guide try to keep this use case in mind.

As an outlook to the auto-formatter discussion: Automatic formatting takes away all discussion about the "right" style in reviews, leading overall to better productivity. These discussions have happened in many languages and projects already, even in OpenTitan, where we, for C and C++, settled on "whatever clang-format produces is the right formatting."

Of course, nothing is ever black and white: style guides can be improved and changed, and since this whole topic is rather new for SystemVerilog, we have the ability to very directly influence the direction the style guide and the formatter might take.

There is no guarantee of success. But it's worth a try, IMO, especially in a project like OpenTitan, where engineers from very different backgrounds come together, need to get up to speed quickly, and then build something sustainable: IP blocks which are not only developed and understood by a single person or a closely-knit team of long-time colleagues, but IP blocks which are good enough to be extended, refactored, and maintained by a group of people over years.

[eunchan]

Having a formatter for SystemVerilog differs from having mandate guidelines in the style guide. I rather have a formatter than this kind of unnecessary tabular discussion, which is not necessary if/ when we have formatter.

Every formatter has exceptions as the software is not perfect. Even tool think the space, line break, are necessary in some part of codes, tweaking it manually may result in better readability. So putting exception in the code is not uncommon as you see in the python code in the OpenTitan repository.

I am not saying that the style guide is useless. Most of the time, it helps to have some kind of uniformity in our codes even many contributors have participated in. I just point out that this tabular is too much. It feels like the guide says "You need to make steps 60cm apart always while walking."

On Apr 23, 2021, at 1:34 AM, Philipp Wagner @.***> wrote:

@eunchan https://github.com/eunchan I can understand the desire to have more formatting freedom (I often enough disagree with style guides!). However, having more than one way of formatting code can lead to tedious discussions in PRs about the "preferred" way (tastes differ!), and prevents automatic formatters from working reliably. I'm therefore trying to standardize on a single way of doing things whenever possible.

We are not enforcing an automated SystemVerilog formatter at the moment (this will be a separate discussion), but the clarifications in the style guide try to keep this use case in mind.

As an outlook to the auto-formatter discussion: Automatic formatting takes away all discussion about the "right" style in reviews, leading overall to better productivity. These discussions have happened in many languages and projects already, even in OpenTitan, where we, for C and C++, settled on "whatever clang-format produces is the right formatting."

Of course, nothing is ever black and white: style guides can be improved and changed, and since this whole topic is rather new for SystemVerilog, we have the ability to very directly influence the direction the style guide and the formatter might take.

There is no guarantee of success. But it's worth a try, IMO, especially in a project like OpenTitan, where engineers from very different backgrounds come together, need to get up to speed quickly, and then build something sustainable: IP blocks which are not only developed and understood by a single person or a closely-knit team of long-time colleagues, but IP blocks which are good enough to be extended, refactored, and maintained by a group of people over years.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/style-guides/issues/48#issuecomment-825496616, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJDG3WQSWVKD7LYMSS5YMLTKEWK3ANCNFSM427OSQUA.

[mdhayter]

I suspect that if the style guide and the formatter are out of step then there is an opening for many time-wasting debates. Long term maintenance is helped by a single style and that means exceptions should be rare (or the formatter should be improved which is probably the case for our python checker).

In this case the only exception I could see to the proposed port formatting is in the case that everything will fit on the same line

  and2 my_gate (.out_o(sig_output), .in1_i(first_input), .in2_i(second_input));

vs

  and2 my_gate (
    .out_o(sig_output),
    .in1_i(first_input),
    .in2_i(second_input)
  );

Can we get that added to verible? If so then I could see us adding to both the style guide and the formatter. If not then we are probably better off with the guide and formatter matching.

[imphil]

@eunchan: As @mdhayter said, having a single "correct" style is required if we want to rely on automatic formatting. As long as we keep human judgement in the loop we will continue to have discussions in pull requests how we should format code. (And make the implementation of the formatter almost impossible: how should it choose between two options?)

That being said, we don't necessarily need to add all rules the formatter implements to the style guide first. Once the formatter is stable enough, we could also turn things around and say "the correct style is whatever the formatter produces" (this is what we do for C/C++ code, for example). However, until the formatter is good enough to use it as reference, we need to have the discussions around preferred style somewhere -- and I find the easiest place to have them is here.

@tomroberts-lowrisc @dsandrs Would you mind weighing in? (The proposed change to the style guide is in PR #50, which also has some additional discussion relevant to this issue.)

Regarding your point @mdhayter on keeping everything on a single line: This would indeed be in line with the rest of our style guide. I filed https://github.com/lowRISC/style-guides/issues/53 to discuss this further.

[tomeroberts]

Thanks for driving this forward @imphil and for all the input and discussion on the thread.

It's a bit of a circular dependency in that we need explicit style guide rules to allow the formatter to make progress, which will then render the explicit style guide rules a bit redundant.

Nevertheless, it looks to me like we have enough consensus on the proposed style to push forward with these updates, and therefore to unlock further progress with the formatter. WDYT @dsandrs ?

[dsandrs]

I'm good with option 1 as well.