verible icon indicating copy to clipboard operation
verible copied to clipboard

comment aligment

Open somyadashora opened this issue 4 years ago • 2 comments

Test case Either the comments should be aligned by formatter or already aligned comments should remain intact. The below code was already aligned.

// Input to the formatter, preferably a reduced test case.
    // some comments
    asome_check = variable__12[2] &                     // seven_7 parts
                  variable__12[7];                      // tage is yellow


Include any options or configuration used. verible-verilog-format --inplace myfile.sv Actual output Formatter shifted the line unnecessarily

// This doesn't look right.
    // some comments
    asome_check = variable__12[2] &                     // seven_7 parts
                      variable__12[7];                      // tage is yellow

Include any possible diagnostic messages from the formatter.

Expected or suggested output Comments should be aligned , it can be done either in a scope like always block or function or task or the scope can be divided into sub-scopes depending if 2 or more than 2 new line characters are there.

// This result would look better from the formatter.
        // this is sub scope 1
        none0nine = (!variable__12[4] & ninenine9) |  // comment 1
                    ( variable__12[4] & nine9nine) |  // comment 2
                    ( eight__8_you & ninenine9) ;     // comment 3


        // two new lines above this is sub scope 2
        one_eight_three = variable__12[1] |                  // comment 4
                          eight__8_rom & variable__12[3] ;   // comment 5

Also if variables are spread across multiple lines in any logical expression as shown the can be aligned, probably shifted right to have easy readability.

verible-verilog-format --align-comments --sub-scope=2 --inplace myfile.sv

Citations to published style guides would help.

somyadashora avatar May 22 '21 13:05 somyadashora

I have seen this as well:

Original code:

/* Transfer the reset signal back to the up domain, used to keep the
 * synchronizers in reset until the core is ready. This is done in order to
 * prevent bogus data to propagate to the register map. */
reg [1:0] up_reset_synchronizer_vector = 2'b11;
assign up_reset_synchronizer = up_reset_synchronizer_vector[0];

/*
 * Synchronize the external core reset to the register map domain so the status
 * can be shown in the register map. This is useful for debugging.
 */
reg [1:0] up_core_reset_ext_synchronizer_vector = 2'b11;
wire up_core_reset_ext;

turn into this via the formatter:

  /* Transfer the reset signal back to the up domain, used to keep the
 * synchronizers in reset until the core is ready. This is done in order to
 * prevent bogus data to propagate to the register map. */
  reg [1:0] up_reset_synchronizer_vector = 2'b11;
  assign up_reset_synchronizer = up_reset_synchronizer_vector[0];

  /*
 * Synchronize the external core reset to the register map domain so the status
 * can be shown in the register map. This is useful for debugging.
 */
  reg [1:0] up_core_reset_ext_synchronizer_vector = 2'b11;
  wire up_core_reset_ext;

which is obviously not as good as it was.

rgetz avatar Mar 21 '22 21:03 rgetz

Issue/task related to @rgetz problem: https://github.com/chipsalliance/verible/issues/1173

mglb avatar Mar 22 '22 12:03 mglb