jeremiah-c-leary / vhdl-style-guide

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

Rules to enforce case on `others` keyword in aggregate assignments #1279

Closed JHertz5 closed 1 week ago

JHertz5 commented 1 month ago

Is your feature request related to a problem? Please describe. I'd like rules to enforce case on the others keyword in aggregate assignments, e.g. correcting


architecture rtl of fifo is

  signal sig : signed := (OTHERS => '0');

begin

  A <=
  (
    1      => '1',
    OTHERS => '0'
  );
  A <= (
         1 => '1',
         OTHERS => '0'
       ) when CONDITION else
       (OTHERS => '0');
  with CONDITION select A <=
    (OTHERS => '1') when X;

  process is

    constant con : signed := (OTHERS => '0');
    variable var : signed := (OTHERS => '0');

  begin

    A <=
    (
      1      => '1',
      OTHERS => '0'
    );
    A <= (
           1 => '1',
           OTHERS => '0'
         ) when CONDITION else
         (OTHERS => '0');
    with CONDITION select A <=
      (OTHERS => '1') when X;

    A :=
    (
      1 => '1',
      OTHERS => '0'
    );
    A := (
           1 => '1',
           OTHERS => '0'
         ) when CONDITION else
         (OTHERS => '0');
    with CONDITION select A :=
      (OTHERS => '1') when X;

  end process;

end architecture rtl;

to


architecture rtl of fifo is

  signal sig : signed := (others => '0');

begin

  A <=
  (
    1      => '1',
    others => '0'
  );
  A <= (
         1 => '1',
         others => '0'
       ) when CONDITION else
       (others => '0');
  with CONDITION select A <=
    (others => '1') when X;

  process is

    constant con : signed := (others => '0');
    variable var : signed := (others => '0');

  begin

    A <=
    (
      1      => '1',
      others => '0'
    );
    A <= (
           1 => '1',
           others => '0'
         ) when CONDITION else
         (others => '0');
    with CONDITION select A <=
      (others => '1') when X;

    A :=
    (
      1 => '1',
      others => '0'
    );
    A := (
           1 => '1',
           others => '0'
         ) when CONDITION else
         (others => '0');
    with CONDITION select A :=
      (others => '1') when X;

  end process;

end architecture rtl;
JHertz5 commented 1 month ago

I has planned to tackle this by creating rules using token_case_in_range_bounded_by_tokens for concurrent, sequential, variable_assignment, constant, signal, and variable, to catch the various places that the token might pop up. However, for concurrent, sequential, and variable_assignment, I would need to handle simple, conditional, and selected assignments. This makes using token_case_in_range_bounded_by_tokens tricky, because I would need to have multiple pairs of start and end tokens, which I don't believe that `token_case_in_range_bounded_by_tokens can currently support.

At the moment, it looks like I'll need to extend token_case_in_range_bounded_by_tokens to handle multiple pairs of start and end token. @jeremiah-c-leary I just wanted to check whether you have any thoughts on whether there's a better approach?

jeremiah-c-leary commented 1 month ago

Evening @JHertz5 ,

I will commit an Excel file called VSG_productions.xlsx which lists all the productions in the 2008 VHDL LRM. It is really handy in finding keywords and which productions they are in. For example, the others keyword is located in the following productions:

Image

I just need to find a good place to put it.

--Jeremy

jeremiah-c-leary commented 1 month ago

I added vsg_productions.csv to docs/theory_of_operation.

--Jeremy

JHertz5 commented 1 month ago

@jeremiah-c-leary This will come in very handy, thanks very much!

JHertz5 commented 1 month ago

I've raised PR #1299 to resolve this issue.

JHertz5 commented 4 weeks ago

FYI @urbite. If you have a chance, please test out my branch and let me know whether it works for you.

urbite commented 3 weeks ago

I pulled this branch and it appears to have fixed almost all of the keyword capitalization instances that have issues filed. I say 'almost' because I haven't yet gone through the issue list to compare.

The testing method was to first convert an VHDL file to lowercase with a corresponding yaml file, then copy that file and convert to uppercase. Then diff the files. A manual process, but quick.

I did find a few other keywords appear to be not yet implemented

@JHertz5 Really excellent work! This is very much appreciated. I do intend to contribute to this project in the future, a first step to continue paring down the list of unimplemented case rules. Just have to find a spare evening to dig in.

JHertz5 commented 3 weeks ago

Hi @urbite.

I pulled this branch and it appears to have fixed almost all of the keyword capitalization instances that have issues filed. I say 'almost' because I haven't yet gone through the issue list to compare.

Fantastic, thanks very much!

I did find a few other keywords appear to be not yet implemented

Thanks for checking these.

@JHertz5 Really excellent work! This is very much appreciated. I do intend to contribute to this project in the future, a first step to continue paring down the list of unimplemented case rules. Just have to find a spare evening to dig in.

Thanks for the kind words! No problem, there's no pressure. If you do find yourself with some spare time, I'm sure any code contributions would be welcome, but, in the meantime, raising issues is also a valuable contribution.

urbite commented 3 weeks ago
  • shared - there is an issue open for this already, https://github.com/jeremiah-c-leary/vhdl-style-guide/issues/1289. I've left this one alone as you said that you might be interested in raising a PR, and this seems like a good introductory issue for you to try. However, if you would like me to raise a PR for it, I'd be happy to do so.

I will commit to raising a PR for this issue. I rarely use this keyword, and suspect it's not widely used. So the priority on fixing this shouldn't be too high.

JHertz5 commented 3 weeks ago

Perfect! Absolutely, I agree that there's no rush. If many people were desperate for that feature, they would have raised the issue before haha.

jeremiah-c-leary commented 1 week ago

Evening @urbite and @JHertz5 ,

@JHertz5 Really excellent work! This is very much appreciated. I do intend to contribute to this project in the future, a first step to continue paring down the list of unimplemented case rules. Just have to find a spare evening to dig in.

Any contribution would be greatly appreciated. @JHertz5 has been really picking up my slack lately. Work has just been too intense for me to really focus on this during the week.

I will get this merged to master.

--Jeremy

JHertz5 commented 1 week ago

Thanks very much!