constant_012 cannot handle aggregates

[staerz]

Environment VSG 2.0.0, fedora 32

Describe the bug The rule constant_012 doesn't treat aggregates properly.

Imagine any constant of type record (and we assume the easiest way here, but one could also imagine nested records) where one element is a std_logic_vector:

  constant AVMM_MASTER_NULL  : avmm_master_t := (
    (others => '0'),
    (others => '0'),
    '0',
    '0'
  );

VSG reports a false violation "Invalid indentation." here. It is probably due to the opening parenthesis (that's the line(s) it complains about.)

To Reproduce Steps to reproduce the behavior:

1. Write code as above
2. Run VSG
3. See error

Expected behavior The code in the given example should pass.

[Malcolmnixon]

I'm having a similar issue with arrays of records. With VSG 2.0.0 I can't find any configuration of bracing in the example below which doesn't stream a bunch of constant_012 Invalid indentation warnings.

    --! Test stimulus
    CONSTANT c_stimulus : t_stimulus_array := 
    (
        ( 
            name      => "Hold in reset       ",
            clk_in    => "0101010101010101",
            rst_in    => "1111111111111111",
            cnt_en_in => "0000000000000000",
            cnt_out   => "0000000000000000"
        ),
        ( 
            name      => "Not enabled         ",
            clk_in    => "0101010101010101",
            rst_in    => "0000000000000000",
            cnt_en_in => "0000000000000000",
            cnt_out   => "0000000000000000"
        )
   );

[jeremiah-c-leary]

Your formatting is definitely better than forcing everything to the left. I must not have the latest version of the LRM. I will see if I can get a copy.

I have re-written the parser to handle constants that cover multiple line. I have not updated the rules yet. Let me try your sample code.

[jeremiah-c-leary]

well, the good news is the parser is assigning the correct object types to all the elements in the constant. This seems more like an indenting issue.

I think to address @Malcolmnixon version we just indent on every open parenthesis, and the de-indent on every close parenthesis.

[jeremiah-c-leary]

I was able to get @Malcolmnixon's version with the correct indent levels:

0 | architecture RTL of FIFO is
None |
1 |     --! Test stimulus
1 |     CONSTANT c_stimulus : t_stimulus_array :=
1 |     (
2 |         (
3 |             name      => "Hold in reset       ",
3 |             clk_in    => "0101010101010101",
3 |             rst_in    => "1111111111111111",
3 |             cnt_en_in => "0000000000000000",
3 |             cnt_out   => "0000000000000000"
2 |         ),
2 |         (
3 |             name      => "Not enabled         ",
3 |             clk_in    => "0101010101010101",
3 |             rst_in    => "0000000000000000",
3 |             cnt_en_in => "0000000000000000",
3 |             cnt_out   => "0000000000000000"
2 |         )
1 |    );

but that doesn't work with @staerz's version. However, the good news is that my re-write is making adjustments like these possible.

[jeremiah-c-leary]

I have not spent much time with the expression part of the constant. I think I need more granularity in my objects to handle it correctly.

[staerz]

Hi @jeremiah-c-leary,

so I think what you should make sure is that VSG can parse and recognise aggregates properly: a (others => '0') or (1 => '0', 2 => '1', others => '-')(or similar) may appear in any place (in a constant or signal definition as well as in the actual architecture as an assignment).

Now the "problem" with longer aggregates is that also they may have two different styles: All in one line (which I think is the more spread) or 1 line per item. The above example would become

(
  1 => '0',
  2 => '1',
  others => '-'
)

(while this is totally valid VHDL, some users might want to keep the parenthesis in the first and last assignment (which I dislike) rather than spending a dedicated line for them as in the code above. And note that I'd actually always go for the one-liner, unless it would become really long)

You'll notice that this is very similar syntax to what @Malcolmnixon is using.

[jeremiah-c-leary]

Hey @staerz and @Malcolmnixon ,

I made an example file that combined both code snippets:

  1
  2 architecture RTL of FIFO is
  3
  4   constant AVMM_MASTER_NULL  : avmm_master_t := (
  5     (others => '0'),
  6     (others => '0'),
  7     '0',
  8     '0'
  9   );
 10
 11   --! Test stimulus
 12     CONSTANT c_stimulus : t_stimulus_array :=
 13     (
 14         (
 15             name      => "Hold in reset       ",
 16             clk_in    => "0101010101010101",
 17             rst_in    => "1111111111111111",
 18             cnt_en_in => "0000000000000000",
 19             cnt_out   => "0000000000000000"
 20         ),
 21         (
 22             name      => "Not enabled         ",
 23             clk_in    => "0101010101010101",
 24             rst_in    => "0000000000000000",
 25             cnt_en_in => "0000000000000000",
 26             cnt_out   => "0000000000000000"
 27         )
 28    );
 29
 30 begin
 31
 32 end architecture RTL;

I ran it against what is current on master and it did not throw constant_017

cp example.orig.vhd example.fixed.vhd;vsg -f example.fixed.vhd --fix
================================================================================
File:  example.fixed.vhd
================================================================================
Phase 7 of 7... Reporting
Total Rules Checked: 431
Total Violations:    0
  Error   :     0
  Warning :     0

However, the formatting is slightly different:

  1
  2 architecture rtl of fifo is
  3
  4   constant avmm_master_null : avmm_master_t := (
  5                                                 (others => '0'),
  6                                                 (others => '0'),
  7                                                 '0',
  8                                                 '0'
  9   );
 10
 11   --! Test stimulus
 12   constant c_stimulus : t_stimulus_array :=
 13     (
 14      (
 15       name      => "Hold in reset       ",
 16       clk_in    => "0101010101010101",
 17       rst_in    => "1111111111111111",
 18       cnt_en_in => "0000000000000000",
 19       cnt_out   => "0000000000000000"
 20       ),
 21      (
 22       name      => "Not enabled         ",
 23       clk_in    => "0101010101010101",
 24       rst_in    => "0000000000000000",
 25       cnt_en_in => "0000000000000000",
 26       cnt_out   => "0000000000000000"
 27       )
 28    );
 29
 30 begin
 31
 32 end architecture rtl;

I have code that aligns based on parenthesis, but you can see how it is not based on indent though.

I wonder if this type of formatting is too difficult. I will have to ponder this a little more.

[staerz]

OK, so one could argue that the opening parenthesis should not be on the line where the constant is defined, but the line after.

I really dislike the idea of having the code that is a subelement of the parenthesis to be indented to the place where the parenthesis starts. I know some people are OK with it but I'm strictly not.

So I would opt for modifying my code from

  constant AVMM_MASTER_NULL : avmm_master_t := (
    (others => '0'),
    (others => '0'),
    '0',
    '0'
  );

to

  constant AVMM_MASTER_NULL : avmm_master_t :=
  (
    (others => '0'),
    (others => '0'),
    '0',
    '0'
  );

In the same spirit, the following, which is similar to what @Malcolmnixon has, could be an allowed syntax:

  constant AVMM_MASTER_NULL : avmm_master_t :=
  (
    (
      others => '0'
    ),
    (
      others => '0'
    ),
    '0',
    '0'
  );

Now I still clearly prefer the before option. The "simple" rule would be that if the "stuff" in parenthesis doesn't contain a comma, then it should all be in 1 single line, and indented with the opening parenthesis. And every comma requires a line break. That should automatically make that the last element is on a single line - and the (/each) closing parenthesis should be on it's own line.

I hope that this rule makes sense to you and would be easy enough to convert into VSG :wink:

[jeremiah-c-leary]

I am wondering if the rule could be as simple as looking at consecutive open and close parenthesis.

For example a single line version of @staerz version would be:

    constant c_stimulus : t_stimulus_array := ((name => "Hold in reset", clk_in => "01", rst_in => "11", cnt_en_in => "00", cnt_out => "00"), (name => "Not enabled", clk_in => "01", rst_in => "00", cnt_en_in => "00", cnt_out => "00"));

if I had the following configuration options:

first_parenthesis_new_line = True
new_line_between_parenthesis = True
indent_lines = True

Then the above code would look like:

    constant c_stimulus : t_stimulus_array :=
      (
        (name => "Hold in reset", clk_in => "01", rst_in => "11", cnt_en_in => "00", cnt_out => "00"), (name => "Not enabled", clk_in => "01", rst_in => "00", cnt_en_in => "00", cnt_out => "00")
      );

If I added in another configuration option:

new_line_after_comma = True

Then the code would be:

    constant c_stimulus : t_stimulus_array :=
      (
        (name => "Hold in reset",
          clk_in => "01",
          rst_in => "11",
          cnt_en_in => "00",
          cnt_out => "00"),
        (name => "Not enabled",
          clk_in => "01",
          rst_in => "00",
          cnt_en_in => "00",
          cnt_out => "00")
      );

Not sure where I was going with this....

It seems there is some basic formatting rule, and then several exceptions to that rule. So....do this, unless x, y or z occurs.

For example: 1) Insert carriage return before open parenthesis except when a) token before is a signal slice or function name 2) Insert carriage return after open parenthesis except when a) token after is others b) token before is a signal slice or function name c) token before is vhdl keyword 3) Insert carriage return before close parenthesis except when a) close parenthesis is closing a signal slice or function name 4) Insert carriage return after close parenthesis except when a) followed by a comma b) followed by a semi colon c) the last close parenthesis 5) Insert carriage return after comma except when a) Inside a function call

[staerz]

Hi @jeremiah-c-leary,

allow me to first point out a general formatting rule (of my preference), before commenting your 5 points:

• The closing parenthesis (the one followed by the semicolon) should have the same indent as the declarative part (in the case above the constant keyword). The reason is very simple: the next declaration is supposed to start on the same indent again (and not, as it would be in your example, one level to the left). It is possible that this is simply my preference, so making this a configuration would be ideal (just like we now have it for the instantiation).

Now replying to your points:

1. I think the reference is the := and then we look if there is a ( following immediately. Then I'd say: Enforce a newline if the next keyword is not others => ... ); (We should not forget about the simplest case where there's a list and it could all be set to constant mylist : t_mylist := (others => '0'); - for this I'd allow it to be in 1 single line.)

2. to 5.: I'd propose a different (maybe less "exceptional") approach: Upon encountering an opening parenthesis, identify the corresponding closing parenthesis. That should be as easy as counting opening parenthesis' against closing ones.

Then, each ( {some code} )[,,;] should be formatted according to the following rules: If opening_parenthesis_new_line = True and {some code} not others => ...:

(
  {some code}
)[,,;]

Also {some code} will have to be indented/new-lined as above (if it contains further blocks of opening and closing parenthesis) and then, depending if new_line_after_comma = True (or maybe call it one_item_per_line) and {some code} is a list (separated by comma), you list one item per line. This configuration could effectively be separated into "last level" and "other level":

constant c_stimulus : t_stimulus_array :=
(
  (name => "Hold in reset", clk_in => "01", rst_in => "11", cnt_en_in => "00", cnt_out => "00"),
  (name => "Not enabled", clk_in => "01", rst_in => "00", cnt_en_in => "00", cnt_out => "00")
);

could be valid if items of the "last level" don't have to be on a single line, while it would look like this if it's set:

constant c_stimulus : t_stimulus_array :=
(
  (name => "Hold in reset",
    clk_in => "01",
    rst_in => "11",
    cnt_en_in => "00",
    cnt_out => "00"),
  (name => "Not enabled",
    clk_in => "01",
    rst_in => "00",
    cnt_en_in => "00",
    cnt_out => "00")
);

There could be a configuration if the opening parenthesis of a block may be on the same line as the first item; and the closing parenthesis on the same line as the last item. This configuration would only apply in the last level of encapsulated parenthesis blocks. The example above assumes this configuration, while it would have to look like this if that configuration is not allowed:

constant c_stimulus : t_stimulus_array :=
(
  (
    name => "Hold in reset",
    clk_in => "01",
    rst_in => "11",
    cnt_en_in => "00",
    cnt_out => "00"
  ),
  (
    name => "Not enabled",
    clk_in => "01",
    rst_in => "00",
    cnt_en_in => "00",
    cnt_out => "00"
  )
);

With these rules, you should cover everything, even if it's a multidimensional array of more than 2 dims.

Now, we did not yet cover alignment. A simple alignment rule for these blocks would be that all => of consecutive lines (comment lines removed, of course) would match up.

Edit: Important note: When parsing for the opening and closing parenthesis, special care must be given to not parse parenthesis of functions! Effectively any item could be set via a function call - and we wouldn't want a function call to be taken into account here - would we? ... Maybe actually we would:

constant c_stimulus : t_stimulus_array :=
(
  (
    name => "Hold in reset",
    clk_in => "01",
    rst_in => "11",
    cnt_en_in => some_function("00", "A", some_constant),
    cnt_out => "00"
  ),
--- vs.:
  (
    name => "Not enabled",
    clk_in => "01",
    rst_in => "00",
    cnt_en_in => some_function(
      "00",
      "A",
      some_constant
    ),
    cnt_out => "00"
  )
);

... I'd actually say the function call should follow the very same rules as defined by one_item_per_line.

[jeremiah-c-leary]

Hey @staerz,

I created a pull request #526, to add the documentation for this change before I start making any changes. When you get a chance could you review and check if I am on the correct track.

Thanks,

--Jeremy

[staerz]

Alright, I did. I think I found a typo, but the rest looks good to me.

The number of possible combinations makes it difficult to make the list of examples complete, but I think you did a very good job.

[jeremiah-c-leary]

Hey @staerz ,

I am considering splitting the restructuring of the constant declaration from the alignment. Restructuring code is using done in phase 1 and alignment in phase 5.

I am getting closer to completing the restructuring. I just need to implement the new_line_after_comma option.

I also added an Ignore method to the options. I updated the doc if you want to check it the changes.

[staerz]

Hi @jeremiah-c-leary,

I think adding an ignore option is a very good idea!

And yes, if it makes things easier, then splitting into 2 phases is not bad at all. That should though then figure in the documentation I would say as one might get confused of "a job half done".

Looking forward to testing your reworked formatter on that.

[jeremiah-c-leary]

I think adding an ignore option is a very good idea!

Serendipity provided that via @BasF0 shortly before I started working on this. Shows the power of community.

[jeremiah-c-leary]

Hey @staerz and @Malcolmnixon ,

I pushed an initial implementation of this feature onto the issue-450 branch. The rule was split into a rule that splits/combines the multiline constant, rule constant_016, and a rule that handles indenting of the resulting structure, rule constant_012.

Both rules have configuration options. Check the configuring multiline structure_rules and configuring_multine_indent_rule for more information.

When you get a chance, could you try this out and give me any feedback.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

thanks for working on this.

It looks pretty good, but I think we missed to consider one thing:

    constant FEB2_LINK_IC_EC : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
    (
      others => (valid => '0', data => (others => '0'))
    );

is what I think would be desirable as construct.

My config for this is

    "constant_016":{
      "align_left":true,
      "first_paren_new_line":true,
      "last_paren_new_line":true,
      "open_paren_new_line":true,
      "close_paren_new_line":true
    },

I'm not yet sure if I have all settings as I wish (maybe I'd actually prefer the first opening parenthesis on the line above), but the edge case here is the others construct. I think that the parenthesis related to that should be ignored by the rules here.

Now, VSG, with the settings as above, wants me to format things like this:

    constant FEB2_LINK_IC_EC : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) := (
      others => (
        valid => '0', data => (
          others => '0'
        )
      )
    );

So I would hope you could implement that (others => <something> ) is excluded from the parenthesis parsing.

[jeremiah-c-leary]

So I would hope you could implement that (others => ) is excluded from the parenthesis parsing.

Should that be configurable or always keep an others statement together?

I was also wondering about slices:

  constant ... := (
     data => (c_first(31 downto 0))
  );

We probably would not want that converted to:

  constant ... := (
     data => (
                     c_first(
                                31 downto 0
                              )
                )
  );

Indenting is off, but you get the idea. it seems this should not be configurable.

[staerz]

Hi @jeremiah-c-leary,

slices is also a good point. Would probably have been my next thing to stumble over :+1:

I agree, that should also just be kept together. No need for configuration I'd say.

... in fact, others is a slice anyway, so you caught the more general case :wink:

[jeremiah-c-leary]

Hey @staerz ,

I was starting to implement the others and slice update and was having some trouble reliably detecting them. So I decided to try implementing the following rule:

All code after an assignment => shall be on the same line.

Expanding on your example:

    constant FEB2_LINK_IC_EC : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
    (
      others => (valid => '0', data => (others => '0')), others => (1 => '0', (others => '1')), c => slice(32 downto 0)
    );

would be transformed to:

    constant FEB2_LINK_IC_EC : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
    (
      others => (valid => '0', data => (others => '0')),
      others => (1 => '0', (others => '1')),
      c => slice(32 downto 0)
    );

Essentially I search for the assignment operator => and then keep track of the number of open and close parenthesis. When the number of parenthesis are not equal then adding newlines after open parenthesis, before close parenthesis and after commas is disabled. When I run into a close parenthesis and the number of open and close are the same I re-enable those checks.

It seems fairly clean, but I wonder about really long assignments.

I pushed this update to the issue-450 branch. Could you test it out and let me know if there are any corner conditions this implementation does not cover? At the moment, this implementation will just add carriage returns. Removal of carriage returns has not been implemented yet.

[staerz]

Hi @jeremiah-c-leary,

I have tested your most recent commit.

Now it would want me to do the following in one example:

  constant AVMM_SLAVE_NULL : t_avmm_slave :=
  (
    (
      others => '0'),
    '0',
    '0'
  );

This is clearly not nice.

The problem with the parenthesis is really for the (others => <anything>) case, there the parenthesis link to the others.

I think you'd have to walk your way from inside out - meaning if you find an (others => <anything>) you must treat it separately.

Note that this is different from others => <something> which might be in an aggregate where you would have something like my_std_logic_vector <= (1 => '1', 4 => 'Z', others => '0');. Now it is actually the same aggregate, except for the latter has others last. So maybe an option to parse is to find (others => ... vs. ( <not others>.

[jeremiah-c-leary]

Hey @staerz ,

I added detecting of others clauses to constant_016 and ran it against all the examples in this thread. When you get a chance, could you check if it is working on your end.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

we're getting there.

I found another fancy parsing in that code:

    constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
    (
      2 => to_multireg(std_logic_vector(G_GBE_MAC
        ), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR
        ), 1 => to_multireg(std_logic_vector(G_GBE_IP
        ), FPGA_REGS_SOURCE_IP_REG_BADDR
        )
    );

This is with the very same settings. If I also enabled the line break after comma, it would require me to line-break in all the 3 lines with a comma in. Obviously, I would only want a line break before 1 => ....

Now I think what your parenthesis closing detection is still missing are function calls. I.e. I don't quite understand why you would expect a line break here, the "(-to-)"-count is fairly off here.

For the initial example it still asks for one line break after closing parenthesis too much:

    constant FEB2_LINK_IC_EC : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
    (
      others => (valid => '0', data => (others => '0')
        )
    );

That seems to be the same thing as with the above example.

On another topic: The next thing to then get into details is the line break after comma. It might be good to have an option to require it for named association, but maybe not require it for positional association.

I.e. if I enable this line break after comma, it would also require me to do this:

  constant GBE_RX_AVST_NULL : t_avst_packet(data(7 downto 0), empty(0 downto 0), error(5 downto 0)) :=
  (
    x"00",
    '0',
    '0',
    '0',
    "-",
    "000000"
  );

In that case, I hope you'd agree, that a line break is not always useful here. On the other hand, you might construct a case where a line break after a certain number of elements would be optically pleasing (i.e. if the line gets too long), so allowing a line break (but not forcing it) would be nice I think.

While for a named association, the following would be pleasant:

    constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
    (
      2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR),
      1 => to_multireg(std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
    );

(So note here that the comma is within a function and should hence not trigger the required line break. But one might add one if the line gets too long:

    constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
    (
      2 => to_multireg(
        std_logic_vector(G_GBE_MAC),
        FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR
      ),
      1 => to_multireg(std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
    );

to give a mixed example.

I think to cover that, one would have to implement a feature to recognise function calls - but that's certainly not for this issue...)

[jeremiah-c-leary]

we're getting there.

I figured this would take several iterations to get right.

I pushed an update to address the extra line breaks in assignments.

[jeremiah-c-leary]

In that case, I hope you'd agree, that a line break is not always useful here. On the other hand, you might construct a case where a line break after a certain number of elements would be optically pleasing (i.e. if the line gets too long), so allowing a line break (but not forcing it) would be nice I think.

Trying to think this through using your example:

  constant GBE_RX_AVST_NULL : t_avst_packet(data(7 downto 0), empty(0 downto 0), error(5 downto 0)) :=
  (
    x"00",
    '0',
    '0',
    '0',
    "-",
    "000000"
  );

We could add the option except_positional_entries to new_line_after_comma. So if that option is set then following code:

  constant GBE_RX_AVST_NULL : t_avst_packet(data(7 downto 0), empty(0 downto 0), error(5 downto 0)) :=
  (
    x"00", '0', '0',
    1 => (others => '0'),
    '0',
    "-", "000000"
  );

would be valid.

[staerz]

@jeremiah-c-leary, you are mixing syntax. Your example is not valid VHDL code :wink:

So you'd either have positional assignment, that would be:

  constant GBE_RX_AVST_NULL : t_avst_packet(data(7 downto 0), empty(0 downto 0), error(5 downto 0)) :=
  (
    x"00",
    '0',
    '0',
    '0',
    "-",
    "000000"
  );

(FYI, this type is defined like that:

  type t_avst_packet is record
    data  : std_logic_vector;
    valid : std_logic;
    sop   : std_logic;
    eop   : std_logic;
    empty : std_logic_vector;
    error : std_logic_vector;
  end record;

so you see that here positional assignment is used. )

Named association would either look like this:

  constant GBE_RX_AVST_NULL : t_avst_packet(data(7 downto 0), empty(0 downto 0), error(5 downto 0)) :=
  (
    data => x"00",
    valid => '0',
    sop => '0',
    eop => '0',
    empty => "-",
    error => "000000"
  );

(for the example above), or like this (in an array):

    constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
    (
      2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR),
      1 => to_multireg(std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
    );

Note that in the array case, you can also use positional assignment, of course, but that's usually not done in order to avoid possible confusion in the direction of the element (of course, the order is very well defined by VHDL, but will the user make it right? :rofl: ).

So what I think I would suggest is to change the option new_line_after_comma from a true/false setting to the following options: enforce, ignore (that could be the default), except_positional.

But the good news already is that your latest commit fixed the reported problem of the parenthesis count and the only thing for now remaining is what we're discussing. :+1:

[jeremiah-c-leary]

But the good news already is that your latest commit fixed the reported problem of the parenthesis count and the only thing for now remaining is what we're discussing. 👍

Nice!!

So what I think I would suggest is to change the option new_line_after_comma from a true/false setting to the following options: enforce, ignore (that could be the default), except_positional.

What about these settings: insert = adds a new line after commas between elements. remove = removes new lines after commas between elements ignore = do not check for new lines after commas insert_unless_positional = adds a new line after commas unless the element is positional

They seem more descriptive than True or False.

I could update all the options to make them consistent.

[staerz]

Hi @jeremiah-c-leary,

when we talk about naming, we talk about consistency, and then it should possibly be add/remove/<others> :wink:

I'm wondering about

allowing a line break (but not forcing it)

I think the general option ignore is good (so that option should actually exist), but I would want to enforce it, except in the positional case - but there I would not want no line break at all, but rather make the line break optional (as it will depend the case).

If your last option (I think we should find a shorter name for it) would contain that, then I'm happy (but it doesn't quite match from my reading).

[jeremiah-c-leary]

Hey @staerz ,

when we talk about naming, we talk about consistency, and then it should possibly be add/remove/ 😉

add versus insert, that makes sense

add = adds a new line after commas between elements. remove = removes new lines after commas between elements ignore = do not check for new lines after commas except_positional = adds a new line after commas unless the element is positional

[staerz]

Hi @jeremiah-c-leary,

yes, insert → add was one point.

The other is what the 4th option would imply. The option that I'm seeking for is that it would add newlines, but in case of positional, it would not enforce anything but ignore the comma actually. So for the naming it could possibly be better called ignore_positional - which, if we would want to be consistent again in our naming, would make the 3rd option be called ignore_all.

Do you think we're having the same understanding?

[jeremiah-c-leary]

Hey @staerz ,

So for the naming it could possibly be better called ignore_positional - which, if we would want to be consistent again in our naming, would make the 3rd option be called ignore_all.

I think so....You can really only do three things: add, remove, or ignore

Using ignore_positional indicates to ignore commas for positional assignments.

Switching ignore to ignore_all seems clearer with the ignore_positional. However, should we then change add to add_all and remove to remove_all?

Do you think we're having the same understanding?

We will get there eventually 😁

[staerz]

Hi @jeremiah-c-leary,

yes, I fully agree, you basically have these 3 options, but then there is this 4th and that breaks this easy concept.

For the naming, I would leave it add and remove (to keep it simple and aligned to the other rules where you'd have add and remove), but would still add the _all suffix to ignore just to show that there are "exceptions" for ignoring and that there's another ignore option that behaves differently.

But if you really feel like add_all and remove_all would boost the understanding, then sure, why not - but then people might actually ask, "and why is there no add_positional and remove_positional?" ... I mean, if that were an option, OK, but I'd say that would be overdoing the job.

[jeremiah-c-leary]

But if you really feel like add_all and remove_all would boost the understanding, then sure, why not - but then people might actually ask, "and why is there no add_positional and remove_positional?" ... I mean, if that were an option, OK, but I'd say that would be overdoing the job.

There is a point at which trying to be specific can make it confusing and cause more questions to be asked. I am okay with these options:

Add Remove Ignore_all Ignore_positional

btw, I just pushed an update implementing the Ignore_all and Ignore_positional for the new_line_after_comma option.

What do you think about capitalization of the options?

If you get a chance, could you check it out. If it looks good then I will go about refactoring the code and updating documentation to prepare it for merging into master.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

I would not capitalise the option names. Python is a key-senstive language and it does not matter in which way you write "true" or "false", but for these named options it matters a lot.

If you start capitalising the options, where would you stop? Would you also then capitalise your rules? Nonono. It is very good (since consistent) like it is now. Let's keep it like that. You'll already have a long list of major and backwards incompatible changes for VSG 3.0.0 - don't over-extend the patience of your users.

I tried your new commit, it seems to work fine, at least it now allows me to set Ignore_positional which then allows

  constant GBE_RX_AVST_NULL : t_avst_packet(data(7 downto 0), empty(0 downto 0), error(5 downto 0)) :=
  (
    x"00", '0', '0', '0', "-", "000000"
  );

which is very nice.

(Edit: BTW, adding a line break at any point is also not raising any warning which is the behaviour that I was expecting, so that works!)

But when I then check the rule I see it doesn't complain on things like that:

    constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
    (
      2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR), 1 => to_multireg(std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
    );

The only complaint is from the line width, but I would expect constant_016 to kick in as well, as there should be a line break after the comma before the 1 =>.

Could you have a look for yourself (maybe add this "negative test" case in your test suite?)?

[jeremiah-c-leary]

Morning @staerz ,

I would not capitalise the option names. Python is a key-senstive language and it does not matter in which way you write "true" or "false", but for these named options it matters a lot.

Agree, not sure why I felt the need to capitalize.

Could you have a look for yourself (maybe add this "negative test" case in your test suite?)?

I will check why my tests did not capture that case.

[jeremiah-c-leary]

Hey @staerz ,

I updated my tests and found two cases where the Ignore_positional prevented new lines after commas in others and assignments.

I pushed an update and will continue my refactor.

If you could validate my fix, I would appreciate it.

Thanks,

--Jeremy

[jeremiah-c-leary]

Hey @staerz ,

As I was updating the documentation, the add and remove values did not seem to match with most of the option names. So I decided switch back to true and false for the new_line_after_comma option. I did lowercase all of the ignore values though.

[staerz]

Hi @jeremiah-c-leary,

from the behaviour something seems wrong now.

In the following example

  constant AVMM_MASTER_NULL  : t_avmm_master :=
  (
    (others => '0'), (others => '0'), '0', '0'
  );

with the ignore_positional setting, VSG wants me to add newlines, 2 times, it didn't do so in the previous version.

Did I miss an option?

[staerz]

PS: I looked at the documentation now and I don't think it's a good idea to have a third option ignore when the type is boolean. By definition, boolean is either true or false. I suggest you either call it boolean-like or maybe remove the type column.

[jeremiah-c-leary]

Did I miss an option?

I must not be counting parenthesis correctly. You would think that would be easy. I will check into it.

[jeremiah-c-leary]

@staerz ,

Is your first example an assignment to signal?

[staerz]

@staerz ,

Is your first example an assignment to signal?

Yes, it's actually a signal, I thought VSG had run a check on it before, but now I'm not sure anymore, so I remove it from my comment.

The dubious behaviour on the (others => .... now remains. And it did work before.

[jeremiah-c-leary]

  constant AVMM_MASTER_NULL  : t_avmm_master :=
  (
    (others => '0'), (others => '0'), '0', '0'
  );

So others is a positional assignment. Seems obvious now.

[jeremiah-c-leary]

how should the following code be formatted?

   constant cons2 : t_type :=
   (others => (valid => '0', data => (others => '0')), others => (1 => '0', (others => '0'));

The others here is an assignment right? so it should be formatted as:

   constant cons2 : t_type :=
   (
     others => (valid => '0', data => (others => '0')),
     others => (1 => '0', (others => '0')
   );

correct?

[staerz]

I'm wondering in how far your example could be correct syntax. And I think it is not. Having others twice seems very odd to me.

I could only make sense of it if both lines were in another set of parenthesis:

   constant cons2 : t_type :=
   (
     (others => (valid => '0', data => (others => '0'))),
     (others => (1 => '0', (others => '0'))
   );

and then it would only make sense if your type is some record of 2 arrays out of which the first is a record and the last an array again.

The reason is that others can only be used once per level.

And as such, this would then be positional.

So yes, if you want a line break after comma, then your example (modulo parenthesis) is correct, but only if you require line break after comma also for positionals.

[jeremiah-c-leary]

Hey @staerz ,

I have this example file:

  1
  2 architecture rtl of fifo is
  3
  4   constant AVMM_SLAVE_NULL : t_avmm_slave :=
  5   ((others => '0'),'0','0');
  6
  7   constant cons1 : t_type :=
  8   ((others => '0'),(1 => '0', others => '1'),(others => '0'));
  9
 10     CONSTANT c_stimulus : t_stimulus_array := (
 11         (
 12             name      => "Hold in reset       ",
 13             clk_in    => "0101010101010101",
 14             rst_in    => "1111111111111111",
 15             cnt_en_in => "0000000000000000",
 16             cnt_out   => "0000000000000000"
 17         ),
 18         (
 19             name      => "Not enabled         ",
 20             clk_in    => "0101010101010101",
 21             rst_in    => "0000000000000000",
 22             cnt_en_in => "0000000000000000",
 23             cnt_out   => "0000000000000000"
 24         )
 25    );
 26
 27   constant FEB2_LINK_IC_EC : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
 28     ((others => (valid => '0', data => (others => '0'))), (others => (1 => '0', (others => '0'))));
 29
 30   constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
 31     (2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR), 1 => to_multireg(std_logic_vector(G_GB 32
 33   constant AVMM_MASTER_NULL  : t_avmm_master :=
 34   ((others => '0'), (others => '0'), '0', '0');
 35
 36 begin
 37
 38 end architecture rtl;

When I run against this configuration:

rule:
  constant_007:
    disable : True
  constant_012:
    align_left : True
    indent_step : 2
  constant_016:
    first_paren_new_line: 'true'
    last_paren_new_line: 'true'
    open_paren_new_line: 'true'
    close_paren_new_line: 'true'
    new_line_after_comma: 'true'
    ignore_single_line: 'true'

I get:

  1
  2 architecture rtl of fifo is
  3
  4   constant avmm_slave_null : t_avmm_slave :=
  5   (
  6     (others => '0'),
  7     '0',
  8     '0'
  9   );
 10
 11   constant cons1 : t_type :=
 12   (
 13     (others => '0'),
 14     (1 => '0', others => '1'),
 15     (others => '0')
 16   );
 17
 18   constant c_stimulus : t_stimulus_array :=
 19   (
 20     (
 21       name      => "Hold in reset       ",
 22       clk_in    => "0101010101010101",
 23       rst_in    => "1111111111111111",
 24       cnt_en_in => "0000000000000000",
 25       cnt_out   => "0000000000000000"
 26     ),
 27     (
 28       name      => "Not enabled         ",
 29       clk_in    => "0101010101010101",
 30       rst_in    => "0000000000000000",
 31       cnt_en_in => "0000000000000000",
 32       cnt_out   => "0000000000000000"
 33     )
 34   );
 35
 36   constant feb2_link_ic_ec : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
 37   (
 38     (others => (valid => '0', data => (others => '0'))),
 39     (others => (1 => '0', (others => '0')))
 40   );
 41
 42   constant mregs_rw_init_fpga_firmware : t_multiregs(2 downto 1) :=
 43   (
 44     2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR),
 45     1 => to_multireg(std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
 46   );
 47
 48   constant avmm_master_null : t_avmm_master :=
 49   (
 50     (others => '0'),
 51     (others => '0'),
 52     '0',
 53     '0'
 54   );
 55
 56 begin
 57
 58 end architecture rtl;

Which has returns after everything, including commas between elements. When I run against this configuration:

rule:
  constant_007:
    disable : True
  constant_012:
    align_left : True
    indent_step : 2
  constant_016:
    first_paren_new_line: 'true'
    last_paren_new_line: 'true'
    open_paren_new_line: 'true'
    close_paren_new_line: 'true'
    new_line_after_comma: 'false'
    ignore_single_line: 'true'

I get this output:

  1
  2 architecture rtl of fifo is
  3
  4   constant avmm_slave_null : t_avmm_slave :=
  5   (
  6     (others => '0'), '0', '0'
  7   );
  8
  9   constant cons1 : t_type :=
 10   (
 11     (others => '0'), (1 => '0', others => '1'), (others => '0')
 12   );
 13
 14   constant c_stimulus : t_stimulus_array :=
 15   (
 16     (
 17       name      => "Hold in reset       ", clk_in    => "0101010101010101", rst_in    => "1111111111111111", cnt_en_in => "0000000"
 18     ), (
 19            name      => "Not enabled         ", clk_in    => "0101010101010101", rst_in    => "0000000000000000", cnt_en_in => "00 "
 20     )
 21   );
 22
 23   constant feb2_link_ic_ec : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
 24   (
 25     (others => (valid => '0', data => (others => '0'))), (others => (1 => '0', (others => '0')))
 26   );
 27
 28   constant mregs_rw_init_fpga_firmware : t_multiregs(2 downto 1) :=
 29   (
 30     2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR), 1 => to_multireg(std_logic_vector(G_GBE 31   );
 32
 33   constant avmm_master_null : t_avmm_master :=
 34   (
 35     (others => '0'), (others => '0'), '0', '0'
 36   );
 37
 38 begin
 39
 40 end architecture rtl;

Where the returns after commas in the constant c_stimulus are removed. It looks really weird though. When I run against this configuration:

rule:
  constant_007:
    disable : True
  constant_012:
    align_left : True
    indent_step : 2
  constant_016:
    first_paren_new_line: 'true'
    last_paren_new_line: 'true'
    open_paren_new_line: 'true'
    close_paren_new_line: 'true'
    new_line_after_comma: 'ignore'
    ignore_single_line: 'true'

I get:

  1
  2 architecture rtl of fifo is
  3
  4   constant avmm_slave_null : t_avmm_slave :=
  5   (
  6     (others => '0'), '0', '0'
  7   );
  8
  9   constant cons1 : t_type :=
 10   (
 11     (others => '0'), (1 => '0', others => '1'), (others => '0')
 12   );
 13
 14   constant c_stimulus : t_stimulus_array :=
 15   (
 16     (
 17       name      => "Hold in reset       ",
 18       clk_in    => "0101010101010101",
 19       rst_in    => "1111111111111111",
 20       cnt_en_in => "0000000000000000",
 21       cnt_out   => "0000000000000000"
 22     ),
 23     (
 24       name      => "Not enabled         ",
 25       clk_in    => "0101010101010101",
 26       rst_in    => "0000000000000000",
 27       cnt_en_in => "0000000000000000",
 28       cnt_out   => "0000000000000000"
 29     )
 30   );
 31
 32   constant feb2_link_ic_ec : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
 33   (
 34     (others => (valid => '0', data => (others => '0'))), (others => (1 => '0', (others => '0')))
 35   );
 36
 37   constant mregs_rw_init_fpga_firmware : t_multiregs(2 downto 1) :=
 38   (
 39     2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR), 1 => to_multireg(std_logic_vector(G_GBE))
 40   );
 41
 42   constant avmm_master_null : t_avmm_master :=
 43   (
 44     (others => '0'), (others => '0'), '0', '0'
 45   );
 46
 47 begin
 48
 49 end architecture rtl;

Where returns are added except after commas. If I run with this configuration:

rule:
  constant_007:
    disable : True
  constant_012:
    align_left : True
    indent_step : 2
  constant_016:
    first_paren_new_line: 'true'
    last_paren_new_line: 'true'
    open_paren_new_line: 'true'
    close_paren_new_line: 'true'
    new_line_after_comma: 'ignore_positional'
    ignore_single_line: 'true'

I get:

  1
  2 architecture rtl of fifo is
  3
  4   constant avmm_slave_null : t_avmm_slave :=
  5   (
  6     (others => '0'), '0', '0'
  7   );
  8
  9   constant cons1 : t_type :=
 10   (
 11     (others => '0'), (1 => '0', others => '1'), (others => '0')
 12   );
 13
 14   constant c_stimulus : t_stimulus_array :=
 15   (
 16     (
 17       name      => "Hold in reset       ",
 18       clk_in    => "0101010101010101",
 19       rst_in    => "1111111111111111",
 20       cnt_en_in => "0000000000000000",
 21       cnt_out   => "0000000000000000"
 22     ),
 23     (
 24       name      => "Not enabled         ",
 25       clk_in    => "0101010101010101",
 26       rst_in    => "0000000000000000",
 27       cnt_en_in => "0000000000000000",
 28       cnt_out   => "0000000000000000"
 29     )
 30   );
 31
 32   constant feb2_link_ic_ec : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
 33   (
 34     (others => (valid => '0', data => (others => '0'))), (others => (1 => '0', (others => '0')))
 35   );
 36
 37   constant mregs_rw_init_fpga_firmware : t_multiregs(2 downto 1) :=
 38   (
 39     2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR),
 40     1 => to_multireg(std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
 41   );
 42
 43   constant avmm_master_null : t_avmm_master :=
 44   (
 45     (others => '0'), (others => '0'), '0', '0'
 46   );
 47
 48 begin
 49
 50 end architecture rtl;

Where only the named association constant has returns added after commas between elements.

I believe that covers all the cases. Is there anything wrong with the output or am I missing something?

btw, I pushed an update which these examples were generated from.

I also updated the options to take strings. So 'true', 'false', 'ignore', and 'ignore_positional'.

[staerz]

Hi @jeremiah-c-leary,

I think it's very good to have a file that contains a collection of examples!

Now constant feb2_link_ic_ec looks still very strange to me and I think the structure doesn't match the definition, as the outer-most type is an array.

Would you mind correcting this to the following:

 36   constant feb2_link_ic_ec : t_avst_streams(G_QSFP1_RX_W - 1 downto 0)(data(3 downto 0)) :=
 37   (
 38     others => (valid => '0', data => (others => '0')),
 39   );

What concerns the different results in different configurations: 1) (all true) looks very good, no complaints 2) (new_line_after_comma: 'false') I think there's a copy&paste error as line numbers 19 and 31 don't show up properly and this is also where I'd say that the format is not nice. Other than that it looks good as well. 3) (new_line_after_comma: 'ignore') Also here I think there's a c&p mistake in line 40. Other than that, OK. 4) (new_line_after_comma: 'ignore_positional') Looks good.

I have installed your latest update and checked it again.

What I think we should add in your test cases are also examples with too many line breaks like this one:

    constant MREGS_RW_INIT_FPGA_FIRMWARE : t_multiregs(2 downto 1) :=
    (
      2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR),
      1 => to_multireg(
std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
    );

If you run against that, nothing will change (at least I didn't get any complaint). But as you can imagine, I would really want VSG to complain here. Now one can argue that it should always be allowed to put an additional line break in order to break long lines (as you might run into a conflict with length_001, but then indentation should be checked.

The question now is: Would you agree and sth. be done here?

In the given case, I would expect the indentation simply to be 2 more spaces (w.r.t. to the previous line start) and that's it (not aligned with the =>!)

Other than that, I'd say we're close to conclusion!

[jeremiah-c-leary]

Hey @staerz ,

What concerns the different results in different configurations:

(all true) looks very good, no complaints (new_line_after_comma: 'false') I think there's a copy&paste error as line numbers 19 and 31 don't show up properly and this is > also where I'd say that the format is not nice. Other than that it looks good as well.

There was an error in the copying of the file. I "fixed" the mistake just to show where the return occured.

(new_line_after_comma: 'ignore') Also here I think there's a c&p mistake in line 40. Other than that, OK.

Agree. I "fixed" the mistake just to show where the return occured.

(new_line_after_comma: 'ignore_positional') Looks good.

That is awesome!!

Now constant feb2_link_ic_ec looks still very strange to me and I think the structure doesn't match the definition, as the outer-most type is an array.

Would you mind correcting this to the following:

I updated my example file to accordingly. Unfortunately, the rule starting failing because it could not tell the difference between the others in the AVMM_SLAVE_LINK and the feb2_link_ic_ec constants. I had to rethink my approach and ended up rewriting the open_paren_new_line, close_paren_new_line and new_line_after_comma options. I want to think they are more robust now.

What I think we should add in your test cases are also examples with too many line breaks like this one:

Let me add in the ability to remove carriage returns within an assignment so it stays on one line. Unless we want the option to split the line based on line length. That would require another option: wrap_lines. Not sure how messy that could be. Might not be too hard...hmmm

If you run against that, nothing will change (at least I didn't get any complaint).

When I ran your example with the extra return, VSG aligned the second line to the opening parenthesis:

 49   constant mregs_rw_init_fpga_firmware : t_multiregs(2 downto 1) :=
 50   (
 51     2 => to_multireg(std_logic_vector(G_GBE_MAC), FPGA_REGS_SOURCE_MAC_ADDRESS_BLK_BADDR),
 52     1 => to_multireg(
 53                       std_logic_vector(G_GBE_IP), FPGA_REGS_SOURCE_IP_REG_BADDR)
 54   );

The indenting is handled by constant_012, which has an option align_left with a boolean value. I interpreted align_left to apply to the first line and then everything indents based on parenthesis afterwards. I can see how align_left could be interpreted as everything aligns left, but I would like the option to include aligning to parenthesis. Maybe I could add another option: align_to_paren which would indent to the column of the parenthesis.

[jeremiah-c-leary]

Hey @staerz ,

I just pushed an update for this issue where I added another option assign_on_single_line. In my tests I have this code:

  1
  2 architecture rtl of fifo is
  3
  4   constant cons1 : t_type := (
  5     1 => func1(std_logic_vector(G_GEN), G_GEN2), 2 => func1(std_logic_vector(G_GEN3), G_GEN4));
  6
  7   constant cons1 : t_type := (
  8     1 => func1(std_logic_vector(G_GEN), G_GEN2),
  9     2 => func1(
 10  std_logic_vector(G_GEN3), G_GEN4)
 11   );
 12
 13   constant cons1 : t_type := (
 14     1 => func1(
 15 std_logic_vector(G_GEN
 16 ), G_GEN2),
 17     (others => '0'),
 18     2 => func2(std_logic(
 19   G_GEN), G_GEN2));
 20
 21 begin
 22
 23 end architecture rtl;

When I run with all the options enabled I get:

  1
  2 architecture rtl of fifo is
  3
  4   constant cons1 : t_type :=
  5  (
  6     1 => func1(std_logic_vector(G_GEN), G_GEN2),
  7  2 => func1(std_logic_vector(G_GEN3), G_GEN4)
  8  );
  9
 10   constant cons1 : t_type :=
 11  (
 12     1 => func1(std_logic_vector(G_GEN), G_GEN2),
 13     2 => func1( std_logic_vector(G_GEN3), G_GEN4)
 14   );
 15
 16   constant cons1 : t_type :=
 17  (
 18     1 => func1(std_logic_vector(G_GEN), G_GEN2),
 19     (others => '0'),
 20     2 => func2(std_logic(  G_GEN), G_GEN2)
 21  );
 22
 23 begin
 24
 25 end architecture rtl;

You have to ignore the indenting as that is done with rule constant_012.

When you get a chance, could you give a spin and see if it works for you.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

I pulled the latest changes and, ignoring the indentation, it works as expected now.

Also I think the added option assign_on_single_line is useful (although I think I would have gone with true/false rather than true/ignore but fine).

Now, I think constant_016 is done. Should we start talking about constant_012 now?

BTW: I think you should rebase your branch for this issue to master in order to pick up the other changes as well (I always do that locally before installing).