lowRISC / style-guides

lowRISC Style Guides
Creative Commons Attribution 4.0 International
345 stars 122 forks source link

[sv] How to format module instantiations that fit in a single line? #53

Open imphil opened 3 years ago

imphil commented 3 years ago

In the SystemVerilog style guide, should we require module instantiations which fit on a single line to do so? (We're trying to come down to a single way of formatting certain constructs, so this question is either-or.)

Option 1:

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

vs

Option 2:

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

We already prefer (or allow) the use of Option 1 in other occasions, so it would be consistent with that (e.g. https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#begin--end).

I guess the main use case would be the instantiation of SV modules modeling gates, clock buffers, or other tiny modules.

@hzeller Is this something Verible has an opinion on right now?

CC @mdhayter @eunchan @msfschaffner @tomroberts-lowrisc @dsandrs @GregAC @rswarbrick @sriyerg @mwbranstad

(Split out from #48)

sriyerg commented 3 years ago

+1 for option 1 if it fits; though, I am used to not adding any space between my_gate and ( (similar to invoking functions).

mwbranstad commented 3 years ago

since this is in the opinion department, I suppose can't hurt to give one. I tend to like option 2 since it seems more readable to me. Option 1 is more terse, but that reason does not necessarily make it better. So if we are looking for a value-added direction, that is my preference between the two options.

rswarbrick commented 3 years ago

As Philipp wrote, we have to choose one or the other if we want a canonical formatting. I'd probably go for option 2 to avoid spurious diffs when port lists change.

msfschaffner commented 3 years ago

I also lean towards option 2, since I think it is more readable.

GregAC commented 3 years ago

I like the ability to use option 1 where it makes sense, e.g. if you're doing multiple instantiations of some simple primitive it can make it a lot more readable, e.g. so you had some mux_onehot module

mux_onehot baz_mux(.in(baz_mux_in), .out(baz_mux_out), .sel(baz_mux_sel));
mux_onehot bar_mux(.in(bar_mux_in), .out(bar_mux_out), .sel(bar_mux_sel));
mux_onehot foo_mux(.in(foo_mux_in), .out(foo_mux_out), .sel(foo_mux_sel));

But I think we want to strictly enforce it one way or the other? My example above is fairly niche and @rswarbrick makes a good point about diff stability, plus I think there's scenarios where enforcing the single line will make it less readable (imagine a bunch of instantiations that aren't all the same module so don't like up nicely like my example). So I'd probably go for always multiline if we can't allow a designer choice here.