Reformat variable_assignment_004

[staerz]

Same as #499 but for variable assignments respectively.

[jeremiah-c-leary]

Hey @staerz,

I did a simple copy of concurrent_009 to variable_assignment_009 on the issue-549 branch.

When you get a chance, could you check it out.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

I don't know what it is, but running this version on my code base makes VSG run into what seems to be an infinite loop.

I also do get some spooky output like the following:

Error: Unexpected token detected while parsing package_body @ Line 51, Column 39 in file components/fpga/context/functions.vhd
       Expecting : end
       Found     : "1 to length"
None
Error: Unexpected token detected while parsing package_declaration @ Line 91, Column 10 in file components/fpga/context/fpga_if.vhd
       Expecting : end
       Found     : "yet another string"
None

Here the related pieces of code:

...
end package functions;

--! Definition of functions
package body functions is

  --! reminder:
  --! type String is array (positive range <>) of character;
  --! and in particular, the order is "1 to length"
  --!
  --! The ordering is done such that the right most character of the string
  --! appears on the lowest 4 bits of the std_logic_vector.

  function to_slv (a : string) return std_logic_vector is
...

and

...
package fpga_if is
...
  --! @brief Generic array of string.
  --! @details Please see detailed description for #t_slv_vector.
  --!
  --! The actual size of the array is selected upon initialization, e.g.
  --! @verbatim
  --!  constant MY_STRING_ARR : t_string_vector(2 downto 0) :=
  --!  (
  --!    "first string",
  --!    "another string",
  --!    "yet another string"
  --!  );
  --! @endverbatim
  type t_string_vector is array (natural range <>) of string;
...

end package fpga_if;

--! Definition of functions
package body fpga_if is
...

The output is spooky in so far as it is a comment which is being parsed and complained about. I think comments should be entirely ignored.

Possibly important: VHDL-2008 allows multi-line comments via /* ... */ - I don't know in how far the big frameworks for firmware support it. I still don't use it, just like none of the developers in my team (as far as I can tell), but this might give an extra dimension to think about for your parser!

I can unfortunately not tell you exactly what causes this infinite loop, but what I get as output is the following:

^CProcess ForkPoolWorker-10:
Process ForkPoolWorker-3:
Process ForkPoolWorker-4:
Process ForkPoolWorker-19:
Process ForkPoolWorker-6:
Process ForkPoolWorker-8:
Process ForkPoolWorker-11:
Process ForkPoolWorker-18:
Process ForkPoolWorker-17:
Process ForkPoolWorker-13:
Process ForkPoolWorker-16:
Process ForkPoolWorker-9:
Process ForkPoolWorker-15:
Process ForkPoolWorker-2:
Process ForkPoolWorker-12:
Traceback (most recent call last):
  File "/usr/lib/python3.8/multiprocessing/pool.py", line 851, in next
    item = self._items.popleft()
IndexError: pop from an empty deque

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/vsg", line 11, in <module>
    load_entry_point('vsg==3.0.0.dev20', 'console_scripts', 'vsg')()
  File "/usr/local/lib/python3.8/dist-packages/vsg-3.0.0.dev20-py3.8.egg/vsg/__main__.py", line 145, in main
    for tResult in pool.imap(f, enumerate(commandLineArguments.filename)):
  File "/usr/lib/python3.8/multiprocessing/pool.py", line 856, in next
    self._cond.wait(timeout)
  File "/usr/lib/python3.8/threading.py", line 302, in wait
    waiter.acquire()
KeyboardInterrupt

it seems though to be happening in my fpga_if.vhd which I attach here: fpga_if.txt

(This assumption is based on the order of files VSG would parse on master...)

I am using the exact same VSG configuration as I previously provided you here, except disabling the few new rules which spit some error (when running on the latest master (3.2.2-50-g62a86502)): procedure_200, block_comment_001, function_201 to _204, package_400, procedure_100, procedure_201 to _204, variable_assignment_004.

I hope this will help to identify the problem. Let me know how it goes or if you need any further feedback.

[jeremiah-c-leary]

Hey @staerz,

I was able to track down the issue. A double quoted string in a comment was not being considered a comment. I have pushed an update.

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

thanks for your fix, yes, VSG doesn't complain anymore and also doesn't get stuck.

I noted though that your latest update is on master while the update for this issue before was on a dedicated branch. Even if I cherry-pick things together, VSG now still complains about my variable assignments (as given in the aforementioned fpga_if.vhd).

I think the reason is that it's not just a simple "concurrent" statement for a variable, but a structured assignment of a record type variable. So it goes more in the direction of #539.

[jeremiah-c-leary]

So looking at this snippet:

851     ret := (
852       data  => (others => '-'),
853       valid => '0',
854       sop   => '0',
855       eop   => '0',
856       empty => (others => '0'),
857       error => (others => '0')
858     );

I think the following structure rules would apply:

1. carriage return after first parenthesis
2. carriage return before closing parenthesis
3. carriage return after comma

and the following alignment rules would apply:

1. align =>, except for others =>

Would you agree?

[staerz]

Hi @jeremiah-c-leary,

in general, yes, I agree to your proposal.

The point though is on a more global level: This construct clearly depends on the type of the variable (which could similarly be a signal, or even a constant). This structure is due to its record-typeness. Note that it could also be a nested record type and these rule should apply to each level of a nested record.

So if I were you, what I would try is to define some core rules on structures like these. Only as a second step (or layer of implementation, if you want so), I would think about giving them a signal, variable or constant flavour. Maybe you could even entirely disentangle them such that the first becomes a "structural" rule set, while the others are simply checking if e.g. for a variable, the "<variable_name> := <begin_of_some_structure>" is done correctly (i.e. the spaces left and right of the ":=" or using indent and alignment. Then, subsequent lines would simply check for the "structural" rule set. This could be applied on constants, signals and variables and would be the most consistent, I think. Also, if you'd implement a fix or extension (to this structural rule set), you'd only have to touch one place.

By now I have already forgotten, but I think that for some language aspect you had already implemented rules that would do this structural rules. I would simply review that and generalise them into a structural rule set for the other cases (variable, signal, constant).

It may well be that you'd need one set for declaration and one for assignment, but I think that's fair.

Let me know what you think about the proposal.

[jeremiah-c-leary]

Hey @staerz ,

Since it has been a year since we last touched this issue, and the code base has evolved since then, I would like to reset this discussion.

For variable assignments it looks like I am missing a structural rule like concurrent_011 or constant_016. The rule variable_assignment_004 is similar the same as rule concurrent_003, however I am missing a similar rule like concurrent_009 for variable assignments. For reference, concurrent_003 addresses concurrent_simple_signal_assignment while concurrent_009 addresses concurrent_conditional_signal_assignment.

The same is true for the sequential rules, so when we resolve this then issue #699 will essentially be resolved.

Would you have a couple of example snippets of code I could reference?

Regards,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

Since it has been a year since we last touched this issue, and the code base has evolved since then, I would like to reset this discussion.

Totally agreed. I'm not sure if I remember myself what we did a year ago.

Would you have a couple of example snippets of code I could reference?

I have basically 2 types of explicit deactivations of variable_assignment_004:

    ret := (
      data  => (others => '-'),
      valid => '0',
      sop   => '0',
      eop   => '0',
      empty => (others => '0'),
      error => (others => '0')
    );

          v_l0_word :=
            l0a_i.data & l0a_i.valid & l0id_i.data &
            l0id_i.valid & tt_l0_i.data & tt_l0_i.valid & "01" &
            std_logic_vector(resize(unsigned(bcid_i.data), G_BCIDS_W_SPEC)) &
            bcid_i.valid & bcid_i.sop & bcid_i.eop;

So the first one is on record types (we have the rule for signals, but not for variables), the second is to be able to split long assignments to multiple lines.

[jeremiah-c-leary]

Hey @staerz,

For your first example with the record assignment, we did add rule signal_017 but that covered constraints in the signal declaration. I might be able to adapt it. There is also the option of using rule constant_016, which might be a better fit.

btw, it looks like the link to the configuration page is incorrect in the constant_016 documentation. I will have to fix it.

If constant_016 will work, then I can use the existing constant_012 rule to perform the indenting.

I will have to update the rule variable_assignment_004 to not check variable assignments with records. That should be easy.

So I will have two new rules, one for structure and one for alignment.

For your second example, are you looking to have long lines automatically split?

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

we did add rule signal_017

Yes, it was my idea to have what we created for signals also available for variables and constants. That is the intention of this issue.

I had in mind that you could generalise and abstract these rules and apply them to these 3 kinds of elements (signal, variable, constant) as in the end, it's the same structural description. What the rules then are called in the end doesn't matter too much, but at best they should be consistent (such as their names).

So what I would recommend at this point is to revisit what there is for signals, then to abstract it and finally to create a set of rules for signals (already existing), variables and constants following this generalised description.

I hope that this makes sense to you.

[staerz]

are you looking to have long lines automatically split

PS: No, currently I would get the following error:

  variable_assignment_004   | Error      |         91 | Indent with 21 space(s)
  variable_assignment_004   | Error      |         92 | Indent with 21 space(s)
  variable_assignment_004   | Error      |         93 | Indent with 21 space(s)
  variable_assignment_004   | Error      |         94 | Indent with 21 space(s)

So it's about the indent: I don't what VSG to ask me to indent it this way:

          v_l0_word :=
                       l0a_i.data & l0a_i.valid & l0id_i.data &
                       l0id_i.valid & tt_l0_i.data & tt_l0_i.valid & "01" &
                       std_logic_vector(resize(unsigned(bcid_i.data), G_BCIDS_W_SPEC)) &
                       bcid_i.valid & bcid_i.sop & bcid_i.eop;

Instead, the indent should just be the next level of indent (but not aligned with the :=).

VSG should not make any requirements on where line breaks are in long assignments (other then those we had defined based on record structure according to configuration).

[jeremiah-c-leary]

Morning @staerz ,

PS: No, currently I would get the following error:

There are configuration options for variable_assignment_004 which will give you the desired indent:

$ vsg -rc variable_assignment_004
{
  "rule": {
    "variable_assignment_004": {
      "indentStyle": "spaces",
      "indentSize": 2,
      "phase": 5,
      "disable": false,
      "fixable": true,
      "severity": "Error",
      "align_left": "no",
      "align_paren": "yes"
    }
  }
}

Unfortunately, it looks like the documentation does not indicate the align_left and align_paren options are available. If you ever want to check the available options for a rule use the -rc option. I will update the documentation to provide a link to a configuration page.

Hmm...I wonder if I could create a test that would check if a link exists in the documentation if there are more than the first six options shown above.

VSG should not make any requirements on where line breaks are in long assignments (other then those we had defined based on record structure according to configuration).

I agree. Where to break the line is very subjective and it would be frustrating to have VSG work against you.

I had in mind that you could generalise and abstract these rules and apply them to these 3 kinds of elements (signal, variable, constant) as in the end, it's the same structural description. What the rules then are called in the end doesn't matter too much, but at best they should be consistent (such as their names).

Agree.

There is a rule class which I extend into a base rule class which I then extend into an individual rule class. When a new rule is required and a base_rule class exists, then adding a new rule is easy.

For example, the inheritence chain of constant_016 looks like this:

     rule <- multiline_structure <- constant_016

So if multiline_structure will work for variable assignments, then I just extend multiline_structure:

     rule <- multiline_structure <- variable_assignment_???

and change which tokens it operates on.

--Jeremy

[jeremiah-c-leary]

Hey @staerz ,

I added the following rules:

• variable_assignment_007
• variable_assignment_008
• variable_assignment_400
• variable_assignment_401

The 007 and 008 are structure rules while the 400 and 401 are alignment rules.

I pushed an update to branch issue-549a.

When you get a chance, let me know what you think.

--Jeremy

[jeremiah-c-leary]

Hey @staerz ,

I added rules for sequential and concurrent signal assignments. The table below lists all the rules. Rules that are light grey were rules that already existed:

I think that fully covers this issue and issue #699.

Let me know what you think of the updates.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

I'm running the current version over my code.

On the line reported earlier

          v_l0_word :=
            l0a_i.data & l0a_i.valid & l0id_i.data &
            l0id_i.valid & tt_l0_i.data & tt_l0_i.valid & "01" &
            std_logic_vector(resize(unsigned(bcid_i.data), G_BCIDS_W_SPEC)) &
            bcid_i.valid & bcid_i.sop & bcid_i.eop;

I now get:

  variable_assignment_007   | Error      |         90 | Move code after assignment to the same line as assignment.

If I move the first line of the assignment part to the previous line (as the suggested text states), then the error is gone.

Note that in your previous suggestion for variable_assignment_004, it needs to be "align_left": "yes", (and not no) to work out like I want it, but that just a minor remark.

I found an inconsistency in calculating indent, I think:

    mregs_ro_fpga_firmware <= (
      2 => to_mregdata(std_logic_vector(GR_FPGA_VERSION_TAG)),
      1 => to_mregdata(GR_FPGA_NAME)
    );

I have to actively give "align_paren":"no", together with "align_left":"yes",, otherwise I get a complaint which is 2 spaces too much. Apparently the opening parenthesis are counted twice. This shows for the above example with concurrent_401. (Or do I misunderstand the idea of these 2 configuration parameters...?)

Another inconsistency where 2 rules seem to fight each other: I have code like the above

    mregs_ro_fpga_firmware <= (
      2 => to_mregdata(std_logic_vector(GR_FPGA_VERSION_TAG)),
      1 => to_mregdata(GR_FPGA_NAME)
    );

and the following:

  ren_init <=
    '1' when cnt = next_stim - 1 and rst = '0' else
    '0';

I want it to be formatted exactly like that: The parenthesis in the first case on the same line as the assignment operator, and the actual assignment for the 2 cases distributed over 2 lines in the second case.

The relevant configuration for that is

    "concurrent_011":{
      "new_line_after_assign":"yes",
      "ignore_single_line":"yes"
    },
    "concurrent_012":{
      "first_paren_new_line":"no",
      "ignore_single_line":"yes"
    },

which complains about

  concurrent_011            | Error      |        137 | Add return after assignment.

for the first case, while if I change the config to

    "concurrent_011":{
      "new_line_after_assign":"no",
      "ignore_single_line":"yes"
    },
    "concurrent_012":{
      "first_paren_new_line":"no",
      "ignore_single_line":"yes"
    },

then I get a complaint about

  concurrent_011            | Error      |        137 | Move code after assignment to the same line as assignment.

for the second case.

So it looks to me like although concurrent_011 and concurrent_012 are meant to look on distinct pieces of code, they are actually not.

Let me know if I have a misunderstanding.

I'm also a bit confused about the documentation related to concurrent_011 vs. variable_assignment_007 (and sequential_008): They seem to point to different configurations. Please double-check this and let me know what's what.

[jeremiah-c-leary]

Evening @staerz ,

If I move the first line of the assignment part to the previous line (as the suggested text states), then the error is gone.

There is a configuration option for variable_assignment_007 called new_line_after_assign. To match this format:

          v_l0_word :=
            l0a_i.data & l0a_i.valid & l0id_i.data &
            l0id_i.valid & tt_l0_i.data & tt_l0_i.valid & "01" &
            std_logic_vector(resize(unsigned(bcid_i.data), G_BCIDS_W_SPEC)) &
            bcid_i.valid & bcid_i.sop & bcid_i.eop;

Try the following configuration options:

rule:
  variable_assignment_007:
    new_line_after_assign: 'yes'
  variable_assignment_004:
    align_left: 'yes'

So it looks to me like although concurrent_011 and concurrent_012 are meant to look on distinct pieces of code, they are actually not.

I just ran an example where concurrent_003 was trying to run on an array. Let me improve the testing to ensure the rules are only operating on the expected types of code.

I'm also a bit confused about the documentation related to concurrent_011 vs. variable_assignment_007 (and sequential_008): They seem to point to different configurations. Please double-check this and let me know what's what.

I will check it out.

Thanks,

--Jeremy

[jeremiah-c-leary]

Evening @staerz ,

I was able to track down the issue. The base rules I was using for concurrent_011 and concurrent_003 were not excluding arrays.

Your example code snippets:

    mregs_ro_fpga_firmware <= (
      2 => to_mregdata(std_logic_vector(GR_FPGA_VERSION_TAG)),
      1 => to_mregdata(GR_FPGA_NAME)
    );

  ren_init <=
    '1' when cnt = next_stim - 1 and rst = '0' else
    '0';

Will be correctly formatted using this configuration:

rule:
  concurrent_011:
    new_line_after_assign: 'yes'
    ignore_single_line: 'yes'
  concurrent_012:
    first_paren_new_line: 'no'
    ignore_single_line: 'yes'
  concurrent_009:
    align_left: 'yes'
  concurrent_401:
    align_left: 'yes'

I pushed the updates to the branch. Let me know if there are any other issues.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

thanks for the feedback and bugfix. With your suggested settings it works as I want it.

I'm having a question though: I looked up the sequential rules and asked myself why do you need them? I have never seen them complain on my side and they look like the concurrent rules, at least when I look into the documentation. What I'm wondering is if they couldn't be submerged with the concurrent rules.

What I could imagine is that the sequential rules only apply when being within a process, but what is the logic behind to allow a different sort of formatting in that case over when these would be "loose" concurrent statements? I certainly wouldn't want them to be any different and hence 1 (set of) rule(s) should suffice.

Also, I like your table a lot. I think it would be of merit to somehow propagate this information into the doc. What one could do is to have a section "equivalent rules" and then cross-list them. This would allow a user to more easily come up with a consistent rule configuration.

Could you again post a link to preview the documentation? It's a bit hard to review it directly in the pull request changes tab.

[jeremiah-c-leary]

Evening @staerz ,

With your suggested settings it works as I want it.

Woot!!

What I'm wondering is if they couldn't be submerged with the concurrent rules.

What I could imagine is that the sequential rules only apply when being within a process, but what is the logic behind to allow a different sort of formatting in that case over when these would be "loose" concurrent statements? I certainly wouldn't want them to be any different and hence 1 (set of) rule(s) should suffice.

Yeah, there is story behind that.

I agree 100% though. I can merge the sequential and concurrent rules. I would like to make that effort part of my next major release though instead of a minor version. There are additional changes I want to do and it would make sense to do all the large scale changes at the same time.

Also, I like your table a lot. I think it would be of merit to somehow propagate this information into the doc. What one could do is to have a section "equivalent rules" and then cross-list them. This would allow a user to more easily come up with a consistent rule configuration.

I will see if I can find a good place to put it.

Could you again post a link to preview the documentation? It's a bit hard to review it directly in the pull request changes tab.

Let me work on the documentation a bit. It looks like some of it is missing.

--Jeremy

[jeremiah-c-leary]

Morning @staerz ,

Here are the links to the documentation for the rules:

configuring_simple_multiline_structure_rules configuring_array_multiline_structure_rules

configuring_multiline_indent_rules configuring_conditional_multiline_indent_rules

Here is a mapping of rules to their documentation:

I am still trying to figure out where to put the table. I was thinking of collecting the above configuration documentation into a subheading.

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

thanks for pointing to the documentation.

In general, as we did it also for the recent updates, I would prefer to set the name of the option and its possible values in formatted text (should be this).

(I looked a bit over the other configuration pages and they use all very different styles, but that's normal as we just introduced the highlighted version very recently. If you anyway plan on a major release, then I would recommend you review the documentation (maybe start with the configuration pages) in that aspect and I'm happy to review it.)

For your table, I think you can simplify it by putting the different groups next to each other, but I'm actually not sure if that is the best way to present it in the documentation, it would more be for your private documentation and notes.

Actually, the section "Rules Enforcing Multiline Structure Rules" covers that already: From this it is implicitly clear that they act the same way but on different code classes.

Now going over the pages in detail:

• configuring_simple_multiline_structure_rules
  • I would add a note below the example that only the line break is subject of that rule, not the indent of the broken lines. (Maybe linking to the rule that covers the indent in that note could be another idea.)
  • signal_016 shouldn't be listed there, this rule is deprecated (but it should also not be signal_017 either as its configuration is "Configuring Multiline Constraint Rules")
• configuring_array_multiline_structure_rules
  • For the examples I'm wondering what your intention was, it doesn't look systematic to me. Apparently you use a simple code snippet in the beginning and then a more complex (type definition) later on. I think what I would do is the following: Stick to the (very same) more complex example in all cases and go over in 2 chunks of examples:
    • The first chunk gives 1 example per "Method" where the difference between yes and no is illustrated. (Also make sure that "fails" is always the first example, "pass" is the 2nd, they are swapped in a few later examples as of now)
    • The second chunk could be examples of "useful" combinations of all options: We clearly identified different styles that would all have their merit (that's why the different methods exist) and it's possibly of huge help to users to see a full fledged configuration example. (It might be useful to note if some indentation rules kick in in some cases...)
  • The page configuring_use_clause_selected_names.rst is obsolete now and should be deleted. (Only concurrent_011 was still using it but I see you already pointed it to configuring_simple_multiline_structure_rules)
  • constant_012 is wrongly listed (it's using configuring_multiline_indent_rules)
  • signal_016 shouldn't be listed either
  • While looking up the previous bullet, I noted that we both make the same typo: "deprecated" (with "e", not "i"): You may want to grep the doc for it :wink:
  • In signal_017, is it the correct link to the correct configuration?
• configuring_multiline_indent_rules
  • The example Example: align_left "yes", align_paren "no" seems to duplicate itself - only on the second look I see that the idea was that the second constant has false indent (1 instead of 2 spaces) ... maybe just rename this constant to "d_const", and add a note that the default 2 spaces is applied here.
  • Why do you change to a different code example for Example: align_left "yes", align_paren "yes" ... I find that disturbing.
  • You should also cover the case (no, no) for completeness (using the very first code example)
  • Maybe it would be worth noting how the indent looks like if the first parenthesis is on the line with the assignment operator.
  • if_004 is wrongly listed here
  • process_020 doesn't link to this configuration
• configuring_conditional_multiline_indent_rules
  • no special comments (the page is really colourful :wink:) [apart from the general comment on using the highlighting for options and their values]

That's what my picky eyes did encounter this round. Let me know when you want me to have a look again. (I hope that now I figured out how to naviagate the (rendered) documentation on github directly...)

[jeremiah-c-leary]

Good evening @staerz ,

I will take your items above and create a list which I will check off as I complete them.

configuring_simple_multiline_structure_rules

• [x] I would add a note below the example that only the line break is subject of that rule, not the indent of the broken lines. (Maybe linking to the rule that covers the indent in that note could be another idea.)
• [x] signal_016 shouldn't be listed there, this rule is deprecated (but it should also not be signal_017 either as its configuration is "Configuring Multiline Constraint Rules")

configuring_array_multiline_structure_rules

• [x] For the examples I'm wondering what your intention was, it doesn't look systematic to me. Apparently you use a simple code snippet in the beginning and then a more complex (type definition) later on. I think what I would do is the following: Stick to the (very same) more complex example in all cases and go over in 2 chunks of examples:
• [x] The first chunk gives 1 example per "Method" where the difference between yes and no is illustrated. (Also make sure that "fails" is always the first example, "pass" is the 2nd, they are swapped in a few later examples as of now)
• [x] The second chunk could be examples of "useful" combinations of all options: We clearly identified different styles that would all have their merit (that's why the different methods exist) and it's possibly of huge help to users to see a full fledged configuration example. (It might be useful to note if some indentation rules kick in in some cases...)
• [x] The page configuring_use_clause_selected_names.rst is obsolete now and should be deleted. (Only concurrent_011 was still using it but I see you already pointed it to configuring_simple_multiline_structure_rules) <You are correct. I must have wanted to write something, but was distracted and ended up committing the file. I have removed it.>
• [x] constant_012 is wrongly listed (it's using configuring_multiline_indent_rules)
• [x] signal_016 shouldn't be listed either
• [x] While looking up the previous bullet, I noted that we both make the same typo: "deprecated" (with "e", not "i"): You may want to grep the doc for it 😉
• [x] In signal_017, is it the correct link to the correct configuration? <signal_017 is an alignment rule, so I removed it>

configuring_multiline_indent_rules

• [x] The example Example: align_left "yes", align_paren "no" seems to duplicate itself - only on the second look I see that the idea was that the second constant has false indent (1 instead of 2 spaces) ... maybe just rename this constant to "d_const", and add a note that the default 2 spaces is applied here.
• [x] Why do you change to a different code example for Example: align_left "yes", align_paren "yes" ... I find that disturbing.
• [x] You should also cover the case (no, no) for completeness (using the very first code example)
• [x] Maybe it would be worth noting how the indent looks like if the first parenthesis is on the line with the assignment operator.
• [X] if_004 is wrongly listed here
• [X] process_020 doesn't link to this configuration

configuring_conditional_multiline_indent_rules

• [X] no special comments (the page is really colourful 😉) [apart from the general comment on using the highlighting for options and their values]

I will push an update when I complete all the items.

--Jeremy

[jeremiah-c-leary]

Morning @staerz ,

There is a bug in my formatting when I set align_left and align_paren to no.

There is another bug when align_left and align_paren are yes and the first parenthesis is not on the same line as the assignment operator.

I will work on these issues and then continue updating the documentation.

--Jeremy

[jeremiah-c-leary]

Afternoon @staerz ,

I finally pushed an update to the documentation and the two bugs I found. It was touch and go there for awhile as I was working in some old code. I really have to re-write the alignment code and create grand unified alignment rule which I can use for all multiline alignments.

I reworked the format of the documentation to the following:

• Title
• Brief Description
• Option table
• Example configuration
• Code snippet used for examples
• Example results of individual options applied to code snippet
• Example results of multiple options applied to code snippet with configuration
• List of rules for which the documentation applies

The option table has been updated to include everything about the option. The options and their values are now highligheted so they stand out.

I believe I corrected all the of linking issues you pointed out.

Let me know what you think of the documentation updates and if you run into any issues with the code updates I made.

configuring_simple_multiline_structure_rules configuring_array_multiline_structure_rules configuring_multiline_indent_rules configuring_conditional_multiline_indent_rules

Regards,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

first the technical feedback: A run of the most recent dev version of VSG on my code base still works fine, no unexpected reports.

Second, feedback on the documentation:

• configuring_simple_multiline_structure_rules
  • I would also put the assignment operation (in fact any source code) in source code syntax highlighting, i.e. "Code after the assignment operator <= will be moved to the next line."
  • Would it be worth the effort of giving an example of the ignore_single_line method? If you agree, I'd simply add a 3rd assignment to your example that is in one line. Then show how it's being formatted in the 2 following examples and add a note each saying that that assignment will only be formatted when ignore_single_line is no. (This may be a way to be complete without having to give 4 examples (2x2).
• configuring_array_multiline_structure_rules
  • For the method ignore_single_line it would possibly be good to use the exact same wording as description as was used for configuring_simple_multiline_structure_rules
  • Overview table: For move_last_comment the description names "last_paren_new_line" which should be code highlighted
  • Is "Example: last_paren_new_line set to yes and move_last_comment set to yes" actually correctly formatted? Shouldn't the comment now be in its own line prior to the constant definition(s)? [Could possibly just be a copy&paste mistake as the next example looks exactly alike]
  • "Example: Keep all assignments on single line" and "Example: Fully expand expression": All the previous examples were pointing towards it, but it's really good to get 2 examples of complete and consistent configuration! Well done!
  • "Example: Keep all assignments on single line": Is it possible that the 2 subsequent parenthesis in the 1st constant should be split over 2 lines actually to match the 2nd constant's formatting? (last_paren_new_line is set to yes ...)
  • Still one these 2: I'd still prefer to have the comment added before the constant keyword, but that's purely my personal preference ... Maybe to accommodate it, you could use move_last_comment = yes in the very last example (so in 1 of 2 show cases...)
• configuring_multiline_indent_rules
  • The list in the description section of the table seems to be broken. (Or you didn't think of a list in the first place, but for consistency and clarity, I'd always use a list (from now on)
  • "Example: align_left set to yes and align_paren set to no": I think I never really understood the intention behind this way of alignment, just like I don't see it for "Example: align_left set to yes and align_paren set to yes". I think it would be worth being a bit more verbose in the description field of the options table for align_paren. It seems each unbalanced open parenthesis will add 1 level of indent. Is that the idea? If so, just copy&paste this to the description :wink:
  • "Example: align_left set to no and align_paren set to no": It's not clear if VSG would actually do some formatting here if the snipped contained code not following exactly this alignment. I think a good way to show it would be to add a 3rd "misaligned" assignment to the snippet and examples.
• configuring_conditional_multiline_indent_rules
  • "Below is an example of a conditional waveform:": I'm not sure if it's any helpful to have that section here, esp. not since you have a code snippet below the table. Similarly for "Conditional expressions and conditional waveforms are defined as:" I'm not sure if it add value to the reader as the examples are very clear. [And it's actually an inconsistency to the documentation to have that definition and an example of the definition here but not in any other configuration description]
  • Overview table: align_left and align_paren should be using the exact same wording as was used for these methods in configuring_multiline_indent_rules

Alright, that's it. The documentation is really getting better and more consistent each time we iterate!

[jeremiah-c-leary]

Evening @staerz ,

first the technical feedback: A run of the most recent dev version of VSG on my code base still works fine, no unexpected reports.

Nice!!

configuring_simple_multiline_structure_rules

• I highlighted <=
• I thought about including ignore_single_line, but was hoping it was obvious enough that I did not need to document it.

configuring_array_multiline_structure_rules

• I used the same language for the ignore_single_line from the previous document
• I highlighted last_paren_new_line in the move_last_comment description
• Removed the comment at the end of the first VHDL statement. The comment also existed on the first line.

"Example: Keep all assignments on single line" and "Example: Fully expand expression": All the previous examples were pointing towards it, but it's really good to get 2 examples of complete and consistent configuration! Well done!

It helps to step back and think about what the documentation should look like. Then we can get a good content flow. I find that when I code first and then document that the documentation suffers. However, when I document first and then code the code is better.

• Updated the output for "Example: Keep all assignments on single line" from actually running on the code snippet with the configuration.

Still one these 2: I'd still prefer to have the comment added before the constant keyword, but that's purely my personal preference ... Maybe to accommodate it, you could use move_last_comment = yes in the very last example (so in 1 of 2 show cases...)

I am a little unsure of your comment. The move_last_comment would only move the comment up a line, but not before the constant keyword. Could you elaborate?

configuring_multiline_indent_rules

• Fixed list in table

"Example: align_left set to yes and align_paren set to no": I think I never really understood the intention behind this way of alignment, just like I don't see it for "Example: align_left set to yes and align_paren set to yes". I think it would be worth being a bit more verbose in the description field of the options table for align_paren. It seems each unbalanced open parenthesis will add 1 level of indent. Is that the idea? If so, just copy&paste this to the description 😉

That is a much better description for the option. I updated both the yes and no descriptions.

"Example: align_left set to no and align_paren set to no": It's not clear if VSG would actually do some formatting here if the snipped contained code not following exactly this alignment. I think a good way to show it would be to add a 3rd "misaligned" assignment to the snippet and examples.

I hoping it would be obvious that the code would not change, but I see your point. I updated the snippet to misalign the code.

configuring_conditional_multiline_indent_rules

"Below is an example of a conditional waveform:": I'm not sure if it's any helpful to have that section here, esp. not since you have a code snippet below the table. Similarly for "Conditional expressions and conditional waveforms are defined as:" I'm not sure if it add value to the reader as the examples are very clear. [And it's actually an inconsistency to the documentation to have that definition and an example of the definition here but not in any other configuration description]

I agree, and would rather not add LRM entries to the other configuration documents.

Overview table: align_left and align_paren should be using the exact same wording as was used for these methods in configuring_multiline_indent_rules

I copied the content for align_left and align_paren.

The documentation is really getting better and more consistent each time we iterate!

100% agree. Most people do not comment on my documentation. I do appreciate the effort you take to review it and make suggestions. I remember it took us awhile to finally nail down the formatting for the multiline constraints, but it was definitely worth the effort.

I pushed the latest updates. Let me know what you think.

Regards,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

I am a little unsure of your comment. The move_last_comment would only move the comment up a line, but not before the constant keyword. Could you elaborate?

It was my understanding that a trailing comment would be moved ahead of the declaration by this method, but now I learn that it just moves up 1 line. I have to be honest: I don't see the sense of that, but I'm sure you have your reasoning.

No matter what the method actually does, given that there are 2 very complete configuration examples given, I would still favour to have the 2nd example to take the opposite setting for this method than the 1st just for the sake of illustration.

(And if I could convince you that the comment should be moved to the very beginning instead, then I'm happy to see this behavioural change. I.e. I don't know if the comment "in the middle of the declaration" would properly be identified by documentation tools. I strongly doubt it.)

That is a much better description for the option. I updated both the yes and no descriptions.

:+1:

I updated the snippet to misalign the code.

Great, now it's much clearer what actually happens!

configuring_conditional_multiline_indent_rules:

• align_paren has a code formatting error in the description.

I do appreciate the effort you take to review it and make suggestions.

Thanks. That's the least I can do.

Now, believe it or not, when seeing the code formatting error in the description of align_paren, I spotted something else: The descriptions in general (over the 4 pages we're treating here) is inconsistent in using itemised phrases or full sentences. I.e. some start with a lower case (and end with no dot at the end), some start with upper case and end with dot, but there are also the other two variants around.

I would suggest to go for upper case + dot in the end. Although most (if not all in these 4 pages) descriptions here are a single phrase, my experience is that sooner or later you'll find a case where you'd want to add another sentence (a real dot followed by a capital letter) and that then blows up the otherwise (consistently) lower-cased documentation.

But this is now the 3rd level of reviewing, and it's a bare appeal to your expectation of consistency. I hope you see that :wink:

[jeremiah-c-leary]

Evening @staerz ,

It was my understanding that a trailing comment would be moved ahead of the declaration by this method, but now I learn that it just moves up 1 line. I have to be honest: I don't see the sense of that, but I'm sure you have your reasoning.

The option was added to handle the following case:

  constant c_stimulus : t_stimulus_array :=
  (
    option_1 => True,   -- Enable option 1
    option_2 => False,  -- Enable option 2
    option_3 => True);  -- Enable option 3

So the comment belongs with the option_3 assignment and not at the end of the line if the last parenthesis was moved to the next line:

  constant c_stimulus : t_stimulus_array :=
  (
    option_1 => True,   -- Enable option 1
    option_2 => False,  -- Enable option 2
    option_3 => True
  );  -- Enable option 3

However, I just realized that I know nothing about the context of the comment. The last comment could just as easily refer to the entire constant:

  constant c_stimulus : t_stimulus_array :=
  (
    option_1 => True,
    option_2 => False,
    option_3 => True
  );  -- Create stimulus vector

In which case your desire to move it before the constant declaration would be a better solution:

  -- Create stimulus vector
  constant c_stimulus : t_stimulus_array :=
  (
    option_1 => True,
    option_2 => False,
    option_3 => True
  );

I have rule comment_011 which will perform the action (hopefully on multiline arrays). However, the same issue remains, I do not know the context of the comment.

So these rules are relying on the user to configure them correctly. There could be inconsistencies in a code base where the comment should be moved and where it should not be moved. Interesting dilemma. It almost seems as if I should not have implemented the move_last_comment option.

No matter what the method actually does, given that there are 2 very complete configuration examples given, I would still favour to have the 2nd example to take the opposite setting for this method than the 1st just for the sake of illustration.

I add the yes option to the last example and updated the output.

align_paren has a code formatting error in the description.

I missed that last time. It is updated.

I would suggest to go for upper case + dot in the end. Although most (if not all in these 4 pages) descriptions here are a single phrase, my experience is that sooner or later you'll find a case where you'd want to add another sentence (a real dot followed by a capital letter) and that then blows up the otherwise (consistently) lower-cased documentation.

This is a reasonable request and a good guideline going forward. I have updated all of the options to be upper case + dot at the end.

But this is now the 3rd level of reviewing, and it's a bare appeal to your expectation of consistency. I hope you see that

Of course. I like consistency, but I tend to not be consistent. Which is one of the reasons for writing VSG. Now I just need to write tests against my documentation to ensure they meet these new standards. :laugh:

Here are the updated docs: configuring_simple_multiline_structure_rules configuring_array_multiline_structure_rules configuring_multiline_indent_rules configuring_conditional_multiline_indent_rules

Let me know what you think.

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

The option was added to handle the following case

OK, now I see.

I just tested my hypothesis on a quick example with Doxygen and it is as I was anticipating: Not working like this! In fact, yes, Doxygen supports comments at the end of the line, but only if the entire definition is in one line, so as soon as you start spanning it over multiple lines (as we try to achieve here), the trailing comment, even if it would remain the absolute trailing one which worked before the spanning, doesn't work anymore. If an element is scattered over multiple lines, then the comment must go first. (Doxygen is very sensitive to line breaks.)

That's my experience with Doxygen. Now there are other documentation tools out there and one would have to look into them.

From that, my preference should be obvious: Take any comment that appears at the end of a line (which gets scattered over multiple lines by the new VSG rule) and put it ahead of the element it comments (possibly retaining the order in case of comments on multiple lines). There will possibly be minor exceptions, but the users would find that I think.

On the other hand, one may also just have a regular comment in the code which is (intentionally) not picked up by the documentation tool and then one would possibly want to keep these comments untouched in general.

So tricky question. Maybe the best is to do nothing at all?! (Perhaps adding a comment that comments would remain untouched or something in that case ...)

If you feel like this needs a thinking (= issue) of its own, then I'd say fine and we can simply omit (= deactivate) the move_last_comment option for now.

I add the yes option to the last example and updated the output.

Except that you forgot the very last line where it should also have been moved one line up, shouldn't it?

I missed that last time. It is updated.

:+1:

I have updated all of the options to be upper case + dot at the end.

:+1:

Now I just need to write tests against my documentation.

:rofl:

Apart from the previously discussed:

• configuring_simple_multiline_structure_rules
  • :+1:
• configuring_array_multiline_structure_rules
  • "Example: open_paren_new_line set to no": I think you mentioned it before: The trailing comment of the 1st constant definition is missing. I think it should be put back for consistency.
  • "Example: close_paren_new_line set to no" : Same.
  • "Example: new_line_after_comma set to no": Same.
• configuring_multiline_indent_rules
  • :+1:
• configuring_conditional_multiline_indent_rules
  • :+1:

[jeremiah-c-leary]

Afternoon @staerz ,

So tricky question. Maybe the best is to do nothing at all?! (Perhaps adding a comment that comments would remain untouched or something in that case ...)

I am coming to the conclusion that VSG should have minimal rules regarding comments. Essentially just indenting and alignment. VSG should not adjust the placement of the comment because it does not know the context of the comment.

If you feel like this needs a thinking (= issue) of its own, then I'd say fine and we can simply omit (= deactivate) the move_last_comment option for now.

I agree the move_last_comment option should be disabled. I will disable it in the rule and update the documentation.

That may address the final issues in the configuring_array_multiline_structure_rules document.

--Jeremy