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

Add rules to require **begin** keyword for generate statements #1240

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

larshb
Copy link
Contributor

@larshb larshb commented Oct 6, 2024

As discussed in #1239 I think these would be a nice addition to the rules.

I wanted to add an accompanying case_generate_alternative_001, but this was a bit more tricky to figure out.

I guess we'd need a new base-rule e.g. insert_token_right_of_token_if_it_does_not_exist_before_possible_tokens to list the following two tokens as possible end tokens

  • vsg.token.case_generate_alternative.when_keyword
  • vsg.token.case_generate_statement.end_keyword

Should there be separate rules for the following?

  • elsif condition generate -> elsif condition generate begin
  • else generate -> else generate begin

larshb added 2 commits October 6, 2024 17:29
This rule checks for the existence of the **begin** keyword.
This rule checks for the existence of the **begin** keyword.
@larshb
Copy link
Contributor Author

larshb commented Oct 7, 2024

The codeclimate-issues contradicts pre-commit styling and complaints about intentional duplication of the format/docstrings in the two rules.

@JHertz5
Copy link
Contributor

JHertz5 commented Oct 7, 2024

Hi @larshb.

The codeclimate-issues contradicts pre-commit styling and complaints about intentional duplication of the format/docstrings in the two rules.

Don't worry about the codeclimate stuff, it throws errors for most PRs. As long as you've taken a look and solved issues where you can, you can ignore the remaining warnings.
Similar statement from the owner of the repository: #1217 (comment)

@jeremiah-c-leary
Copy link
Owner

Morning @larshb and @JHertz5 ,

Don't worry about the codeclimate stuff, it throws errors for most PRs.

I need to disable the codeclimate checks. There were useful earlier in the development, but now they seem to be too noisy to be useful.

@larshb , I fear implementing this feature will be a little more difficult due to nesting generics. Could you add some nested generates in your test cases? Consider nesting for_generate, if_generate, and case_generate. You could then use the same input for the three versions (for, if and case) of the rule.

Looking at the productions for the

if_generate_statement ::=
generate_label :
  if [ alternative_label : ] condition generate
    generate_statement_body
  { elsif [ alternative_label : ] condition generate
    generate_statement_body }
  [ else [ alternative_label : ] generate
    generate_statement_body ]
  end generate [ generate_label ] ;

each generate_statement_body could have a begin keyword:

generate_statement_body ::=
  [ block_declarative_part
begin ]
  { concurrent_statement }
[ end [ alternative_label ] ; ]

So you could end up with the following code:

example_if:
if enable_fifo generate
    begin
elsif disable_fifo generate
    begin
else generate
    begin
end generate;

...or if you add in the end keywords:

example_if:
if enable_fifo generate
    begin
    end;
elsif disable_fifo generate
    begin
    end;
else generate
    begin
    end;
end generate;

Another issue to keep in mind is the begin keyword must separate the block_declarative_part and any concurrent_statements. Although this might take care of itself because the begin keyword is required if there is a block_declarative_part.

I think if you expand the test input we can better assess whether the rule as implemented provides the correct output in multiple scenarios.

btw, thanks for the pull request.

--Jeremy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants