Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal and variable assignments are not aligned #562

Closed
imd1 opened this issue Apr 28, 2021 · 11 comments
Closed

Signal and variable assignments are not aligned #562

imd1 opened this issue Apr 28, 2021 · 11 comments
Assignees
Labels

Comments

@imd1
Copy link
Contributor

imd1 commented Apr 28, 2021

Environment
VSG 3.1.0, Centos 7.6

Describe the bug
When a process uses signals and variables, their assignments statements are not vertically aligned

Expected behavior
I would expect them to be aligned

Screenshots
Here's an example of VSG fixed code illustrating the problem:

  begin

    if (rising_edge(s_refclk_p)) then
      if (tx_reset_done = '0' or s_adc_aresetn = '0') then
        sysrefcount := 0;
        s_tx0_sysref <= '0';
      else
        if (sysrefcount = 63) then
          if (DAC_SUBCLASS /= 0) then
            s_tx0_sysref <= '1';
          end if;
          sysrefcount := 0;
        else
          s_tx0_sysref <= '0';
          sysrefcount := sysrefcount + 1;

@imd1 imd1 added the bug label Apr 28, 2021
@imd1
Copy link
Contributor Author

imd1 commented Jun 21, 2021

@jeremiah-c-leary : there are separate rules for signals and variables. What's the best way forward? Create a new rule or update an existing rule?

@imd1
Copy link
Contributor Author

imd1 commented Oct 1, 2021

@jeremiah-c-leary: what do you think about my question above?

@jeremiah-c-leary
Copy link
Owner

Hey @imd1,

That is a good question. We have moved most of the alignment rules "up" the hierarchy. For example, architecture_026 so we could align :'s across multiple types of declarations. I would think it would apply here also.

So we could depricate sequential_005 and variable_assignment_005 and create an architecture rule. Maybe a procedure and function rule also?

Would we always want to align the <= and the :=?

@imd1
Copy link
Contributor Author

imd1 commented Oct 4, 2021

Hey @imd1,

That is a good question. We have moved most of the alignment rules "up" the hierarchy. For example, architecture_026 so we could align :'s across multiple types of declarations. I would think it would apply here also.

So we could depricate sequential_005 and variable_assignment_005 and create an architecture rule. Maybe a procedure and function rule also?

Would we always want to align the <= and the :=?

My gut feeling is yes we would

@imd1
Copy link
Contributor Author

imd1 commented Oct 5, 2021

I am planning to have a go at this ticket when I next get time...are you OK with that?

@jeremiah-c-leary
Copy link
Owner

I am planning to have a go at this ticket when I next get time...are you OK with that?

Sure, I would take a look at architecture_029. It might be exactly what you would need.

@imd1
Copy link
Contributor Author

imd1 commented Oct 7, 2021

Having seen your solution to #571, I wonder whether it would be more consistent to update sequential_005 in the same way as subprogram_body_400 and deprecate variable_assignment_005?

@jeremiah-c-leary jeremiah-c-leary added this to To do in Release 3.3.0 via automation Oct 8, 2021
@jeremiah-c-leary
Copy link
Owner

Having seen your solution to #571, I wonder whether it would be more consistent to update sequential_005 in the same way as subprogram_body_400 and deprecate variable_assignment_005?

I would deprecate both sequential_005 and variable_assignment_005 and create a process rule. This is the reasoning:

  1. sequential rules should apply within signal assignments
  2. variable_assignment rules should apply within variable assignments
  3. rules that apply across more than one type of "rule" should exist in the rule category that contains what is being operated on
    a) For example, subprogram_body contains both signal and variable assignments, and needs to operate on both of them.
  4. subprogram_body_400 already covers functions and procedures

This is something that has evolved as VSG was worked on. There are still some rules that should be moved according to the above statements.

In fact, sequential rules should be renamed to signal_assignment.

You could probably copy subprogram_body_400 and make it a process rule and change oStart and oEnd.

@jeremiah-c-leary jeremiah-c-leary removed this from To do in Release 3.3.0 Oct 15, 2021
@jeremiah-c-leary jeremiah-c-leary added this to To do in Release 3.4.0 via automation Oct 15, 2021
@jeremiah-c-leary jeremiah-c-leary self-assigned this Oct 17, 2021
@jeremiah-c-leary jeremiah-c-leary moved this from To do to In progress in Release 3.4.0 Oct 17, 2021
jeremiah-c-leary added a commit that referenced this issue Oct 17, 2021
Adding rule to align signal and variable assignment operators.

  1)  Updated documentation
  2)  Updated tests
      a)  Removed deprecated tests
      b)  Added process tests
  3)  Updated rules
      a)  Deprecated signal_005
      b)  Deprecated variable_assignment_005
      c)  Added rule process_400
@jeremiah-c-leary
Copy link
Owner

Hey @imd1,

Not sure if you were able to get to this, but I pushed an update to this to the issue-562 branch.

When you get a chance could you give it a try and let me know what you think?

Thanks,

--Jeremy

@jeremiah-c-leary jeremiah-c-leary moved this from In progress to User Validation in Release 3.4.0 Oct 17, 2021
@imd1
Copy link
Contributor Author

imd1 commented Nov 2, 2021

Hey @imd1,

Not sure if you were able to get to this, but I pushed an update to this to the issue-562 branch.

When you get a chance could you give it a try and let me know what you think?

Thanks,

--Jeremy

Effectively reviewed pulled request and it looks good from the unit tests - no time to actually try it out, so go for it when you want to

jeremiah-c-leary added a commit that referenced this issue Dec 11, 2021
Adding severity to test JSON output.
jeremiah-c-leary added a commit that referenced this issue Dec 11, 2021
Formatting updates.
jeremiah-c-leary added a commit that referenced this issue Dec 11, 2021
* Updates for issue #562

Adding rule to align signal and variable assignment operators.

  1)  Updated documentation
  2)  Updated tests
      a)  Removed deprecated tests
      b)  Added process tests
  3)  Updated rules
      a)  Deprecated signal_005
      b)  Deprecated variable_assignment_005
      c)  Added rule process_400

* Updates for issue #562

Adding severity to test JSON output.

* Updates for issue #562

Formatting updates.
@jeremiah-c-leary
Copy link
Owner

This has been merged to master and will be part of the 3.4.0 release.

Release 3.4.0 automation moved this from User Validation to Done Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

No branches or pull requests

2 participants