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 rule generate_022 (require keyword **begin** in for generate) #1239

Closed
wants to merge 1 commit into from

Conversation

larshb
Copy link
Contributor

@larshb larshb commented Oct 5, 2024

We may want to require begin-keywords for generate statements.

This rule only adds this for "for generate" statements, and not "if generate" nor "case generate" (yet).

It is disabled by default.

A weird artifact I can't but my finger on is when not configured as disabled by default, pytest requires the following change

diff --git a/tests/styles/base/nested_generates.fixed.vhd b/tests/styles/base/nested_generates.fixed.vhd
index 490c678c..ecc28b03 100644
--- a/tests/styles/base/nested_generates.fixed.vhd
+++ b/tests/styles/base/nested_generates.fixed.vhd
@@ -6,7 +6,7 @@ begin
   g_0 : if true generate
 
     g_1 : if true generate
-      signal sig0  : bit;
+      signal sig0   : bit;
       signal sig00 : bit;
     begin

@JHertz5
Copy link
Contributor

JHertz5 commented Oct 6, 2024

Hi @larshb. I'm not the owner of this project, but I'd like to offer some suggestions:

From the description of the PR, it sounds like you plan to add either similar rules or expand this new rule to enforce begins for case and if generates as well.

Instead of a single rule for all types of generates, I would split this rule up for each different kind of generate (this may be your plan already, it's not clear to me). i auggest this for a couple of reasons.

  • This allows the user to individually disable or enable each rule (they may want begins in their for generates, but not in their case generates).
  • This makes each rule atomic and simple, which makes the code as a whole easier to write, read, and test.

Then, with individual rules each serving only one kind of generate statement, I suggest that they should live in the set of rules for that particular kind of generate statement rather than in the plain generic set of rules.
For example, the rule you've introduced here would become a for_generate_statement rule.

Again, I'm not the owner of the project, feel free to disagree. Just offering my unsolicited opinion haha.

@larshb
Copy link
Contributor Author

larshb commented Oct 6, 2024

Hi @JHertz5! Your feedback is very much appreciated.

I would like to extend the rule(s) for case generate and if generate as well, similar to generate_011.

I see your point, and splitting up a general rule would make the implementation of the rule(s) less complex, and probably the rule-set as a whole less computationally heavy.

I see all of the rules for for_generate_statement, if_generate_statement and case_generate_statement are put in later phases. Should the begin-rules continue that trend in your opinion?

@JHertz5
Copy link
Contributor

JHertz5 commented Oct 6, 2024

Should the begin -rules continue that trend in your opinion?

Short answer: No, I think that you have the rule executing in the correct phase currently.

Long answer: The phase in which a rule executes is driven by what the rule enforces. For example, rules that enforce indentation are executed in phase 4, and rules that enforce case are executed in phase 6. This is why the rules that you mentioned run in later phases, because they enforce things like indentation and case.

So I would just let the base rule that you're using dictate the phase. Your rule should run early; if, for example, it runs in phase 5, then it runs after the rule that enforces the indentation (generate_006) which means that the new begin's indentation may be incorrrect.

@larshb
Copy link
Contributor Author

larshb commented Oct 6, 2024

This PR is replaced by #1240

@larshb larshb closed this Oct 6, 2024
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.

2 participants