search

jeremiah-c-leary / vhdl-style-guide

Style guide enforcement for VHDL
GNU General Public License v3.0
189 stars 38 forks source link

Reformat variable_assignment_004 #549

Closed staerz closed 2 years ago

staerz commented 3 years ago

Same as #499 but for variable assignments respectively.

jeremiah-c-leary commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 " := " 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Hey @staerz ,

I added the following rules:

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 commented 2 years ago

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:

Image

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

Let me know what you think of the updates.

Thanks,

--Jeremy

staerz commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

Image

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 commented 2 years ago

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:

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 commented 2 years ago

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

configuring_array_multiline_structure_rules

configuring_multiline_indent_rules

configuring_conditional_multiline_indent_rules

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

--Jeremy

jeremiah-c-leary commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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:

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

jeremiah-c-leary commented 2 years ago

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

configuring_array_multiline_structure_rules

"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.

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

"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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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:

jeremiah-c-leary commented 2 years ago

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

jeremiah-c-leary commented 2 years ago

Evening @staerz ,

I have removed the move_last_comment option and pushed an update to the documentation.

This could be the one!!

configuring_array_multiline_structure_rules

Let me know what you think.

Regards,

--Jeremy

staerz commented 2 years ago

Hi @jeremiah-c-leary,

I have taken a look at the updated documentation (and the related commit) and from that I can tell that we're all good now!

So yes, that's the one! Go for it!

Always a pleasure to bring long stories like this to a successful end!

jeremiah-c-leary commented 2 years ago

Evening @staerz ,

So yes, that's the one! Go for it!

Woot!!

I will merge this to master and I am planning to do a release this weekend.

Always a pleasure to bring long stories like this to a successful end!

Agree, and I believe this closes the last of your issues.

Regards,

--Jeremy

staerz commented 2 years ago

Hi @jeremiah-c-leary,

I am planning to do a release this weekend.

Very good. I also saw the announcement. Well deserved break!

I believe this closes the last of your issues.

Yes, I checked, that was the last one. I double-checked against my internal list and found another one which also falls a little into the category of what we discussed here, but it's not worth bugging you before you leave.