Extended configurability

[LarsAsplund]

I'm introducing vsg in a project which has a history of mixed styles. Some have used alignment, some have not. Moving forward, we will remove all alignment and use --fix to enforce that in git hooks. While doing that, we found the following issues (in order of priority):

1. When running --fix, we managed to remove spaces after => in port and generic mappings but not spaces before =>. This means that these lines are only partly "de-aligned" after --fix. For example, my_port => some_signal,.
2. If we have a hierarchical reference (external name), for example << signal dut.foo : signal >>, we have not been able to remove the spaces before :.
3. This is more an indentation issue but if we have a long condition to an (els)if statement and break the line into several, it would be nice if the second line could be indented. Like this:

   elsif condition_a and
   condition_b then

[jeremiah-c-leary]

Afternoon @LarsAsplund

I am currently away from a computer for the next three weeks, but I can answer some of these questions.

1. It appears there is no rule to force spacing before the assignment operator in port and generic maps. This is a little puzzling to me as I would expect one to exist. I will work on adding a rule when I return sometime after June 2nd.

2. Have you checked the configuration of rule signal_006? There is an option to set the spacing to 1. The default is at least one space to allow for alignment.

3. Have you checked the configuration of rule if_009? It handles alignment of multiline conditions in if expressions. If you disabled all the alignment rules using via a group, You can enable that rule invidually.

Regards,

--Jeremy

[ru551n]

I'll respond here since I am the "active" user of this here at the moment.

1. It appears there is no rule to force spacing before the assignment operator in port and generic maps. This is a little puzzling to me as I would expect one to exist. I will work on adding a rule when I return sometime after June 2nd.

Appreciate it! :)

2. Have you checked the configuration of rule signal_006? There is an option to set the spacing to 1. The default is at least one space to allow for alignment.

We have that one enabled, it doesn't seem to fix the colon around the signal keyword when used in hierarchical references.

3. Have you checked the configuration of rule if_009? It handles alignment of multiline conditions in if expressions. If you disabled all the alignment rules using via a group, You can enable that rule invidually.

We have that rule enabled, with align_left: yes and align_paren: no, it still seems to put the next row of statements on the same level.

[jeremiah-c-leary]

Morning @ru551n ,

We have that one enabled, it doesn't seem to fix the colon around the signal keyword when used in hierarchical references

I reviewed the rule and it should work with hierarchical references. Could you post the output of the following command:

vsg -rc signal_006

We have that rule enabled, with align_left: yes and align_paren: no, it still seems to put the next row of statements on the same level

Maybe I misunderstood the original question. Is the issue the indenting is not working or the structure of the expression (adding or removing carriage returns) the issue?

--Jeremy

[ru551n]

I reviewed the rule and it should work with hierarchical references. Could you post the output of the following command:

λ vsg -rc signal_006 -c .vsg.yaml
{                                
  "rule": {                      
    "signal_006": {              
      "indent_style": "spaces",  
      "indent_size": 2,          
      "phase": 2,                
      "disable": false,          
      "fixable": true,           
      "severity": "Error",       
      "number_of_spaces": 1      
    }                            
  }                              
}                                

[jeremiah-c-leary]

Afternoon @ru551n ,

It appears the rule is configured correctly for a single space. I will need to get to a computer before I can debug any further. I will be back from vacation on June 2nd and will take a look at these issues shortly after.

Regards

--Jeremy

[jeremiah-c-leary]

Morning @ru551n ,

I have added two new rules to enforce whitespace before port map and generic map assigment operators to branch issue-1170. This should address the first issue. When you get a chance could you check it out on your end and see if it is working for you.

For the second issue, I have the following code snippet:

  1 architecture rtl of fifo is
  2
  3   signal dut.foo      : std_logic;
  4
  5 begin
  6
  7 end architecture rtl;

I used the following configuration file:

  1 "rule": {
  2   "global": {
  3      "disable" : true
  4   },
  5   "signal_006": {
  6      "disable" : false,
  7      "number_of_spaces" : 1
  8   }
  9 }

when I run vsg against the file using the configuration I get the following code:

  1 architecture rtl of fifo is
  2
  3   signal dut.foo : std_logic;
  4
  5 begin
  6
  7 end architecture rtl;

I must be missing something as rule signal_006 seems to be enforcing a single space before the colon. Could you post a snippet that shows the issue you are seeing?

Regards,

--Jeremy

[jeremiah-c-leary]

Afternoon @ru551n ,

Is there a chance you could post a snippet of the third issue? I have my test example and it seems to be working as expected:

  1 architecture rtl of fifo is
  2
  3   signal dut.foo      : std_logic;
  4
  5 begin
  6
  7   PROC_1:process
  8
  9   begin
 10
 11     if condition_a and condition_b then
 12     elsif condition_a and
 13                         condition_b then
 14     end if;
 15
 16   end process PROC_1;
 17
 18 end architecture rtl;

with this configuration:

  1 "rule": {
  2   "signal_006": {
  3      "disable" : false,
  4      "number_of_spaces" : 1
  5   },
  6   "if_002": {
  7     "parenthesis": "remove"
  8   },
  9   "if_009": {
 10     "align_left": true,
 11     "align_paren": false
 12   }
 13 }

yields:

  1 architecture rtl of fifo is
  2
  3   signal dut.foo : std_logic;
  4
  5 begin
  6
  7   proc_1 : process is
  8   begin
  9
 10     if condition_a and condition_b then
 11     elsif condition_a and
 12       condition_b then
 13     end if;
 14
 15   end process proc_1;
 16
 17 end architecture rtl;

I feel I am missing something on the last issue.

--Jeremy

[jeremiah-c-leary]

Morning @ru551n and @LarsAsplund ,

Just wanted to ping you on this issue to see if we can move this forward.

Thanks,

--Jeremy

[ru551n]

Morning @ru551n and @LarsAsplund ,

Just wanted to ping you on this issue to see if we can move this forward.

Thanks,

--Jeremy

Due to some more urgent stuff being requested this has been on hold for a while. Now I am back on it though, so here goes:

1. The two new rules work perfectly, cheers!
2. Aliases as hierarchical references still doesn't work properly, i.e

   alias test is << signal dut.test :     std_logic >>;

   Does not get aligned.

3. You are correct, it does indeed work properly.

[jeremiah-c-leary]

Evening @LarsAsplund ,

So apparently it took me awhile to figure out you were referring to external_signal_names. I updated the parser to detect and parse external_signal_name, external_variable_name, and external_constant_name. I also added rules for external_signal_name. There are several whitespace and a case rule for signal. Could you give this update a try and let me know if it fixes the whitespace issue.

Also, would you like similar rules for external_variable_name and external_constant_name?

Regards,

--Jeremy

[ru551n]

Evening @LarsAsplund ,

So apparently it took me awhile to figure out you were referring to external_signal_names. I updated the parser to detect and parse external_signal_name, external_variable_name, and external_constant_name. I also added rules for external_signal_name. There are several whitespace and a case rule for signal. Could you give this update a try and let me know if it fixes the whitespace issue.

Also, would you like similar rules for external_variable_name and external_constant_name?

Regards,

--Jeremy

Works nicely!

Regarding external constants and variables, it's not something we use commonly, but if it is not too much work it is better to have than not.

Thank you!

-- Sebastian

[LarsAsplund]

@jeremiah-c-leary It works perfectly on external names when used with aliases but not on external names used in a context like this:

probe_signal <= << some.hierarchy : std_logic >>

We use both styles

[jeremiah-c-leary]

Morning @LarsAsplund ,

Regarding external constants and variables, it's not something we use commonly, but if it is not too much work it is better to have than not.

I can add those rules.

We use both styles

I checked the 2008 LRM and the only definition for an external names are the following:

     external_signal_name ::=
         << signal external_pathname : subtype_indication >>

     external_constant_name ::=
         << constant external_pathname : subtype_indication >>

     external_variable_name ::=
         << variable external_pathname : subtype_indication >>

The signal, constant, and variable are not optional., unless there was an update in a later version of the LRM. Any chance I can convince you to add in the missing signal, constant, and variable? I will still need to update the parser to detect external_names as part of signal assignments though.

Regards,

--Jeremy

[jeremiah-c-leary]

Evening @LarsAsplund ,

I added rules for the external_constant_names and external_variable_names.

Regards,

--Jeremy

[jeremiah-c-leary]

Morning @LarsAsplund and @ru551n ,

Just wanted to ping you on this issue to see if we can resolve the discrepancy between LRM and the following code:

probe_signal <= <<    some.hierarchy : std_logic   >>

--Jeremy

[jeremiah-c-leary]

Evening @ru551n and @LarsAsplund ,

Just wanted to ping you on this issue to see if we can move it forward to completion.

Thanks,

--Jeremy

[LarsAsplund]

@jeremiah-c-leary Sorry for the delay.

probe_signal <= <<    some.hierarchy : std_logic   >>

is a typo from my side. I think this issue can be closed but I will make a test run with the latest version to see if I can find any remaining issues.

[LarsAsplund]

One issue I see is if the external name references a signal in a path generated by a generate statement. In those cases the external name will start with some.path.generate_label(i) where i is a number. VSG doesn't like the parenthesis in the the path.

[LarsAsplund]

Other than that I can't see any issues on our code based. If that is fixed, these changes can be merged and released as far as I'm concerned.

[jeremiah-c-leary]

Evening @LarsAsplund,

I will take a look at the condition with the parenthesis.

--Jeremy

[jeremiah-c-leary]

Evening @LarsAsplund ,

I have pushed an update to handle parenthesis. When you get a chance could you check it out.

Thanks,

--Jeremy

[LarsAsplund]

@jeremiah-c-leary Looks good to me! Will you make a release?

[jeremiah-c-leary]

Evening @LarsAsplund,

I will merge this to master and schedule it for release this weekend.

--Jeremy