jeremiah-c-leary / vhdl-style-guide

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

Add rule generate_022 (require keyword **begin** in for generate) #1239

Closed larshb closed 1 month ago

larshb commented 1 month ago

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 commented 1 month ago

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.

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

This PR is replaced by #1240