verible icon indicating copy to clipboard operation
verible copied to clipboard

Formatter Features Request

Open Remillard opened this issue 1 year ago • 3 comments

Two requests are from the issue https://github.com/chipsalliance/verible/issues/2362 about half way down, but cleaned up here as a formal feature request. The third is one I just found today -- might actually be a bug, but it might be more of a feature.

  1. Aligning the default assignments and end-line comments in signal declarations.

Instead of this: Aligns the following at the signal name:

    // Internal registers
    T_REGDATA       reg_temp_result = 16'h0000;  // R
    T_REGDATA       reg_slew_result = 16'h0000;  // R
    T_REGDATA       reg_alert_status = 16'h0000;  // R/RC
    T_REGDATA       reg_configuration = C_REG_CONFIG_INIT;  // R/W
    T_REGDATA       reg_alert_enable = C_REG_ALERT_EN_INIT;  // R/W
    T_REGDATA       reg_tlow_limit = C_REG_TLOW_LIM_INIT;  // R/W
    T_REGDATA       reg_thigh_limit = C_REG_THIGH_LIM_INIT;  // R/W
    T_REGDATA       reg_hysteresis = C_REG_HYST_INIT;  // R/W
    T_REGDATA       reg_slew_limit = C_REG_SLEW_LIM_INIT;  // R/W
    T_REGDATA       reg_unique_id1 = 16'h0000;  // R
    T_REGDATA       reg_unique_id2 = 16'h0000;  // R
    T_REGDATA       reg_unique_id3 = 16'h0000;  // R
    T_REGDATA       reg_device_id = C_REG_DEVICE_ID_INIT;  // R     

Aligning would produce:

     // Internal registers
    T_REGDATA       reg_temp_result   = 16'h0000;              // R
    T_REGDATA       reg_slew_result   = 16'h0000;              // R
    T_REGDATA       reg_alert_status  = 16'h0000;              // R/RC
    T_REGDATA       reg_configuration = C_REG_CONFIG_INIT;     // R/W
    T_REGDATA       reg_alert_enable  = C_REG_ALERT_EN_INIT;   // R/W
    T_REGDATA       reg_tlow_limit    = C_REG_TLOW_LIM_INIT;   // R/W
    T_REGDATA       reg_thigh_limit   = C_REG_THIGH_LIM_INIT;  // R/W
    T_REGDATA       reg_hysteresis    = C_REG_HYST_INIT;       // R/W
    T_REGDATA       reg_slew_limit    = C_REG_SLEW_LIM_INIT;   // R/W
    T_REGDATA       reg_unique_id1    = 16'h0000;              // R
    T_REGDATA       reg_unique_id2    = 16'h0000;              // R
    T_REGDATA       reg_unique_id3    = 16'h0000;              // R
    T_REGDATA       reg_device_id     = C_REG_DEVICE_ID_INIT;  // R     
  1. Permit the formatting of subprograms in a style similar to module. So for example instead of this:
   task spi_write_16(input logic [15:0] sdio_word, output logic sdio, output logic sdio_oe,
                      output logic sclk, output logic cs_n, input integer start_flag,
                      input integer end_flag);

I would like to see:

    task spi_write_16 (
        input  logic [15:0] sdio_word, 
        output logic        sdio, 
        output logic        sdio_oe,
        output logic        sclk, 
        output logic        cs_n, 
        input  integer      start_flag,
        input  integer      end_flag
    );
  1. This one seems to be a bug because it behaves correctly in the module definition but it does not in the body with signal definitions. For unpacked arrays in signal definitions, either bring the unpacked dimension up to the signal name, or align it somewhere nearby. Currently formatting does the following:
    logic    [ 9:0] alarm_hi_reg                                  [16];
    logic    [ 9:0] alarm_lo_reg                                  [16];

And it should be:

    logic    [ 9:0] alarm_hi_reg  [16];
    logic    [ 9:0] alarm_lo_reg  [16];

Admittedly, this is not a good example for aligning them as both names are identical in length, however if they were of variable length, aligning them in closer would be proper.

Thanks!

Remillard avatar Mar 13 '25 19:03 Remillard

The third issue might be caused by a bug that I am also seeing in regards to an interaction between unpacked arrays and comments. It appears that the formatter will align unpacked arrays to the right of any comments in the same grouping of aligned variables. for example, the formatter will currently do the following:

  logic signed [PARAM3-1:0] var_1                                                 [PARAM1-1:0]                          [PARAM2-1:0];  // This code is more spread out than it should be
  logic        [       1:0] var_2_long;  // This comment pushes the unpacked array
  logic                     var_3_even_longer                                     [       1:0];  // So does this comment

when in reality it should be:

  logic signed [PARAM3-1:0] var_1            [PARAM1-1:0] [PARAM2-1:0];  // This code is more spread out than it should be
  logic        [       1:0] var_2_long;  // This comment pushes the unpacked array
  logic                     var_3_even_longer[       1:0];  // So does this comment

MPethick avatar Apr 09 '25 11:04 MPethick

Did you try the various formatting options ? (I am not sure if these are covered here, but I'd have a look at them first).

There are some formatting options that try to infer from the existing style (e.g. if you already have mostl of the lines aligned in columns, it tries to do that); if not, it leaves it as-is as the assumption is that is the desired style. If you want a particular style, you can explicitly choose that.

hzeller avatar Apr 09 '25 18:04 hzeller

I have tried setting the various align parameters to align and it does nothing for me (cannot speak to the other fellow's issue). What I've noticed is that the formatter does not do any secondary or tertiary aligning. It'll align assignments, but anything past that is pretty iffy. So the alignment of unpacked dimensions is sometimes weird, and alignment of default values and EOL comments is also questionable.

In any event, nothing I've tried so far aligns things past the packed dimension and name.

Remillard avatar Apr 09 '25 22:04 Remillard