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

Blank line before if and after end if. #291

Open
m-kru opened this issue Nov 21, 2019 · 26 comments · May be fixed by #788
Open

Blank line before if and after end if. #291

m-kru opened this issue Nov 21, 2019 · 26 comments · May be fixed by #788
Assignees

Comments

@m-kru
Copy link
Collaborator

m-kru commented Nov 21, 2019

rule:

if_031                    |        518 | Add a blank line before the "if"
if_030                    |        530 | Add a blank line after the "end if"

I like these rules. However, it should be possible to configure some exceptions or change how these rules works for example consider this snippet:

  process (clk) is
  begin
    if (rising_edge(clk)) then
      if a < 250 then
        a  <= a + 1;
        b  <= '0';
      else
        a  <= (others => '0');
        b  <= '1';
      end if;
    end if;
  end process;

In such case I want begin line to be before if and end process line to be after end if.

After some reflection, I think the same complaint can be made for other blank line rules.

@jeremiah-c-leary
Copy link
Owner

Just to clarify, you would like the code to be formatted like this:

  process (clk) is
  begin if (rising_edge(clk)) then
      if a < 250 then
        a  <= a + 1;
        b  <= '0';
      else
        a  <= (others => '0');
        b  <= '1';
      end if;
    end if; end process;

@m-kru
Copy link
Collaborator Author

m-kru commented Nov 21, 2019

Yes, but if there is some extra signal assignment on the rising edge, I would like to force following style:

  process (clk) is
  begin 
    if (rising_edge(clk)) then
      extra_signal <= foo;

      if a < 250 then
        a  <= a + 1;
        b  <= '0';
      else
        a  <= (others => '0');
        b  <= '1';
      end if;
    end if;
  end process;

Btw. How did you get colors in you snippet?

@jeremiah-c-leary
Copy link
Owner

jeremiah-c-leary commented Nov 21, 2019

To get the colors you wrap the code in

  """```vhdl"""

  "```"

without the double quotes.

@jeremiah-c-leary
Copy link
Owner

jeremiah-c-leary commented Nov 21, 2019

It seems like there are two requests here:

  1. if there is a rising_edge check directly after a begin then place the check after the begin:

Violation

  process (clk) is
  begin
     if (rising_edge(clk)) then

Fix

  process (clk) is
  begin if (rising_edge(clk)) then

The second is: the first if statement inside a rising edge check should have a blank line above it:

Violation

  if (rising_edge(clk)) then
     extra_signal <= foo;
     extra_sig2 <= foo2;
     if a < 250 then

Fix

  if (rising_edge(clk)) then
     extra_signal <= foo;
     extra_sig2 <= foo2;

     if a < 250 then

The thinking being the rising edge is really just checking for a clock edge while the next if is the real condition you are interested in emphasizing. Correct?

@m-kru
Copy link
Collaborator Author

m-kru commented Nov 21, 2019

Oh no sorry, there is a mistake I have not realized. I do not want begin and if on the same line. I have fixed the snippet. Your second example is closer to what I want to achieve, but is bounded to specific example. Let's leave the if construct and focus generally on all before/after blank line rules. The problem with these rules is that they do not know what is on the line after/before and they always force blank line. I would like this mechanism to be more configurable or 'smart'.

Now lets come back to the if example,:

process (clk)
begin
  if rising_edge(clk) then
    if condition then
      d <= q;
    end if;
  end if;
end process;

In this case I do not want to have blank lines between

begin
  if rising_edge(clk) then
    if condition then

and

    end if;
  end if;
end process;

However if there were any assignment before if or before or after end if I would like vsg to treat them as an error:
Following example:

process (clk)
begin
  if rising_edge(clk) then
    a <= b;
    if condition then
      d <= q;
    end if;
  end if;
  c <= f;
end process;

should generate errors. Something like "Insert blank line between assignment and if block", "Insert blank line after if block, between end if and assignment. The fixed code would look like this:

process (clk)
begin
  if rising_edge(clk) then
    a <= b;

    if condition then
      d <= q;
    end if;

    c <= f;
  end if;
end process;

I would like to be able to force developer to use blank lines to separate logic flow (programming logic flow, not design logic flow, although both are closely related). However it is not possible as vsg knows nothing about program logic. But I think, that with smart blank line rules behavior, one would at least stop for a while and rethink which statements should be grouped.

@jeremiah-c-leary
Copy link
Owner

I think I already added the ability to detect clock processes by checking for the 'event and I think rising_edge and falling_edge. We could add an attribute to classify the line as a programming logic versus design logic. Then we use one of the blank line rules and modify it to check for assignments.

@m-kru
Copy link
Collaborator Author

m-kru commented Nov 22, 2019

Hmm, I like this idea with classifying lines. However I am not sure if programming_logic and design_logic is good division. For example is end if; programming logic? I think we should not hurry with this. It would be good idea to check if something similar exists in linters for other languages.

@jeremiah-c-leary jeremiah-c-leary added this to To do in Release 2.1.0 via automation Jul 11, 2020
@jeremiah-c-leary jeremiah-c-leary removed this from To do in Release 2.1.0 Dec 5, 2020
@jeremiah-c-leary
Copy link
Owner

Hey @m-kru ,

Are you still interested in this issue?

--Jeremy

@m-kru
Copy link
Collaborator Author

m-kru commented Mar 18, 2021

@jeremiah-c-leary Nope.

@Bonusbartus
Copy link

Hi @jeremiah-c-leary,

would it be possible to re-open/revisit this idea? I think the concept would be quite simple.

  1. keep the possibility of the blank line after an "end if;" (or other end *;)
  2. add possibility to skip that blank line if the next token is another end (end if, end case, end process etc)
    the current rule only skips it for nested end if statements

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus,

We can revisit this idea, we just need to define the rules.

Going back to the previous example:

process (clk)
begin

  x <= y;
  if rising_edge(clk) then
    if condition then
      a <= b;
      if condition then
        d <= q;
      end if;
      c <= f;
    end if;
  end if;
  z <= x;

end process;

Is this how you would like it formatted?

process (clk)
begin

  x <= y;

  if rising_edge(clk) then
    if condition then

      a <= b;

      if condition then

        d <= q;

      end if;

      c <= f;

    end if;
  end if;

  z <= x;

end process;

I could also see an option for not inserting blank lines if there is a single line between the if and end if:

process (clk)
begin

  x <= y;

  if rising_edge(clk) then
    if condition then

      a <= b;

      if condition then
        d <= q;
      end if;

      c <= f;

    end if;
  end if;

  z <= x;

end process;

--Jeremy

@jeremiah-c-leary jeremiah-c-leary self-assigned this May 3, 2022
@Bonusbartus
Copy link

Hi @jeremiah-c-leary,

yes, that looks perfect. Although in my case I think I'd switch off the blank line insertion after the if, but that is (and should stay) a different rule.
so in my case it would look (something) like this:

process (clk)
begin
  x <= y;

  if rising_edge(clk) then
    if condition then
      a <= b;

      if condition then
        d <= q;
      end if;
      c <= f;

    end if;
  end if;

  z <= x;

end process;

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus,

Just wanted a clarification on you last snippet with regards to c <= f;. Would you prefer something like:

process (clk)
begin
  x <= y;

  if rising_edge(clk) then
    if condition then
      a <= b;

      if condition then
        d <= q;
      end if;

      c <= f;
    end if;
  end if;

  z <= x;

end process;

It seems to be more consistent not having a blank line above c <= f. Essentially the rules would be:

  1. Blank line after end if unless the next line is an end if, or other ends
  2. Blank line before if unless the previous line is an if

--Jeremy

@Bonusbartus
Copy link

Hi @jeremiah-c-leary,

you are right, that looks/is better. These rules seem to be correct.

thanks!

jeremiah-c-leary added a commit that referenced this issue May 25, 2022
Adding ignore_hierarchy option to rule if_030.

  1)  Added test
  2)  Added rule
@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus ,

I pushed an interim update for this issue to the issue-291 branch.

This update exposed an issue with what constitutes a nested if statement that I need to resolve. Essentially is a nested if statement broken up by a case statement or loop still part of the nested if statement?

   if a = 0 then  -- Hierarchy level 0
      if b = 0 then  -- Hierarchy level 1
           case x is
              when 3 =>
                   if c = 0 then  -- Hierarchy level 0 or 2
                   end if;
            end case;
       end if;
   end if;

There are two new options: allow_end_ifs and allow_end_process for rule if_030.

If you could check them out while I decide what to do with the hierarchy question I would appreciate it.

Thanks,

--Jeremy

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus,

I pushed another update for this to the issue-291 branch. It includes options to adjust the blank line/no blank line depending on what follows the end if.

Could you try it out and let me know what you think.

I just need to go through some refactoring and it will be ready to merge to master.

Thanks,

--Jeremy

@Bonusbartus
Copy link

Hey @jeremiah-c-leary,

Seems to be working nicely. Haven't been able to verify all the possible options, but at least the base functionality, except_end_process and except_end_case seem to work!

Thanks for the additions,

Bart

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus,

except_end_process ... seem to work

How is the rule process_023 working with the new parameters. It seems as if you could configure a conflict such that either rule would always fail. I might need to add an exception in that, and similar rules. Hmmm...maybe it makes more sense to move those options to their corresponding rules instead of in if_030.

I also realized I did not update if_031, so I am not as close to being done as I hoped.

--Jeremy

@Bonusbartus
Copy link

Hey @jeremiah-c-leary
You are right, those two rules conflict. I didn't notice at first because I set the process_023 to:
style: "no_blank_line"

not sure if it is needed to handle exceptions, or if it is something to just configure in such a way that it doesn't conflict?

Bart

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus ,

I apologize, but I lost track of this issue. Reading through the comments it looks like I updated rule if_030, but still need to update if_031. There also seems to be a possible issue with process_023 being in conflict.

Does that sum up the status of this issue?

Thanks,

--Jeremy

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus ,

I updated if_031 to allow blank lines to be removed before the consecutive if statements.

To enable this feature you need to use the following configuration:

rule:
  if_031:
    ignore_hierarchy: False
    except_if_statement: True
    style: 'require_blank_line'

When you get a chance could you give it a try and let me know how it goes?

Thanks,

--Jeremy

@jeremiah-c-leary
Copy link
Owner

Hey @Bonusbartus ,

Just wanted to ping you to see if you were able to try out the latest update.

Thanks,

--Jeremy

@Bonusbartus
Copy link

Bonusbartus commented Oct 18, 2022 via email

@jeremiah-c-leary
Copy link
Owner

Morning @Bonusbartus ,

Just wanted to ping you to see if you had a change to validate the update.

Thanks,

--Jeremy

@Bonusbartus
Copy link

Hi @jeremiah-c-leary
I have been playing around with issue branch 291 and am not sure it is working entirely as I would expect.
some things I noticed:
I get an error on the first if (risign_edge) .. which btw I would also want to exclude as between begin and the if statement nothing happens.
I don't however seem to be able to generate an error on the first else, or on any of the end if.

  p_example : process (clk)
  begin
    if (rising_edge(clk)) then
      if (rst_n = '0') then
        X <= '0';
        y <= '0';
      else
        x <= '0';
        y <= '0';

        if (a = '0' and b = '1') then
          x   <= '1';
          y   <= '1';
        end if;

        if (c = '0' and d = '1') then
          x  <= '0';
          y  <= '1';
        end if;
      end if;
    end if;
  end process;

correct would be (I think)

  p_example : process (clk)
  begin
    if (rising_edge(clk)) then
      if (rst_n = '0') then
        X <= '0';
        y <= '0';

      else
        x <= '0';
        y <= '0';

        if (a = '0' and b = '1') then
          x   <= '1';
          y   <= '1';

        end if;

        if (c = '0' and d = '1') then
          x  <= '0';
          y  <= '1';

        end if;
      end if;
    end if;
  end process;

or

  p_example : process (clk)
  begin
    if (rising_edge(clk)) then
      if (rst_n = '0') then
        X <= '0';
        y <= '0';

      else
        x <= '0';
        y <= '0';

        if (a = '0' and b = '1') then
          x   <= '1';
          y   <= '1';

        end if;
        if (c = '0' and d = '1') then
          x  <= '0';
          y  <= '1';

        end if;
      end if;
    end if;
  end process;

@jeremiah-c-leary
Copy link
Owner

Good Eventing @Bonusbartus ,

Well....it's been awhile but I can finally come back to this issue. Just wanted to ping you check if this is still something you would want me to address before I start working on this again.

Thanks,

--Jeremy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: User Feedback
Development

Successfully merging a pull request may close this issue.

3 participants