jeremiah-c-leary / vhdl-style-guide

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

Rules to align `when` in selected assignments #983

Closed JHertz5 closed 1 year ago

JHertz5 commented 1 year ago

Is your feature request related to a problem? Please describe. Consider the following code:

with mux_sel select addr <=
  c_input_data when 0,
  c_output_data when 1,
  (others => 'X') when others;

wr_en <= c_value when a = '1' else
         '0' when b = '0' else
         c when d = '1' else
         f;

I would like to align the when keywords, i.e.:

with mux_sel select addr <=
  c_input_data    when 0,
  c_output_data   when 1,
  (others => 'X') when others;

wr_en <= c_value when a = '1' else
         '0'     when b = '0' else
         c       when d = '1' else
         f;

As far as I can tell, as of v3.16.0, there are no rules that would enforce the when alignment in either assignment the with statement.

Describe the solution you'd like I would like to have a rule or rules added that would enforce the alignment of the when keyword for selected and conditional assignments. Perhaps this would be a single rule within the when ruleset, or perhaps a rule in each of the selected_assignment and conditional_expression rulesets. Update As @jeremiah-c-leary has kindly pointed out, there are already rules in place that handle conditional assignments. This feature request has been amended to only request a rule for selected assignments.

Describe alternatives you've considered None

Additional context None

jeremiah-c-leary commented 1 year ago

Morning @JHertz5,

I believe the following configuration option handle the following code:

With the following configuration:

  1 rule:
  2   concurrent_009:
  3     align_left : 'no'
  4     align_paren : 'no'
  5     align_when_keywords : 'yes'
  6     wrap_at_when : 'no'
  7     align_else_keywords : 'yes'

Your example code would be fixed as:

  1 architecture rtl of fifo is
  2
  3 begin
  4
  5   with mux_sel select addr <=
  6     c_input_data when 0,
  7     c_output_data when 1,
  8     (others => 'X') when others;
  9
 10   wr_en <= c_value when a = '1' else
 11            '0'     when b = '0' else
 12            c       when d = '1' else
 13            f;
 14
 15 end architecture rtl;

I do not currently have a rule to handle the with statement however.

There is also a conflict with rule conditional_waveforms_100:

================================================================================
File:  fixed.vhd
================================================================================
Phase 2 of 7... Reporting
Total Rules Checked: 290
Total Violations:    2
  Error   :     2
  Warning :     0
----------------------------+------------+------------+--------------------------------------
  Rule                      |  severity  |  line(s)   | Solution
----------------------------+------------+------------+--------------------------------------
  conditional_waveforms_100 | Error      |         11 | Change the number of space(s) between '0' and when to 1
  conditional_waveforms_100 | Error      |         12 | Change the number of space(s) between c and when to 1
----------------------------+------------+------------+--------------------------------------

Not 100% sure the best way to handle this conflict. We could disable the rule using code tags like this:

  1 architecture rtl of fifo is
  2 begin
  3 with mux_sel select addr <=
  4   c_input_data when 0,
  5   c_output_data when 1,
  6   (others => 'X') when others;
  7 -- vsg_off conditional_waveforms_100
  8 wr_en <= c_value when a = '1' else
  9          '0' when b = '0' else
 10          c when d = '1' else
 11          f;
 12 -- vsg_on
 13 end architecture RTL;

Although you will end up with code tags in code.

Regards,

--Jeremy

JHertz5 commented 1 year ago

Hi @jeremiah-c-leary. I apologise! You are absolutely right, the conditional multi-line indent rules completely fulfil my needs on the conditional assignments. The conflict with conditional_waveforms_100 is not a problem for me, I have set that to number_of_spaces: '>=1' so it does not raise any errors. I will amend this feature request to only request a rule for with statements.

jeremiah-c-leary commented 1 year ago

Good Morning @JHertz5 ,

I pushed an update to the issue-983 branch that should allow you align the when keywords in your example. I added rule concurrent_selected_signal_assignment_400 to enforce alignment.

When you get a chance could you try it out and let me know how it works on your end.

Thanks,

--Jeremy

JHertz5 commented 1 year ago

Hi @jeremiah-c-leary. Thank you so much! I have tried the new rule on that branch and it is working for me. I am satisfied that this issue can be closed once this branch is merged.

jeremiah-c-leary commented 1 year ago

Awesome, I will merge this to master.