changed Always to inheret from StructuralStatement #5
Conversation
src/verilogAST.cpp
Outdated
| @@ -200,8 +200,7 @@ std::string Module::toString() { | |||
| // emit body | |||
| for (auto statement : body) { | |||
| module_str += | |||
| variant_to_string<Always *, StructuralStatement *, Declaration *>( | |||
| statement) + | |||
| variant_to_string<StructuralStatement *, Declaration *>(statement) + | |||
| ";\n"; | |||
There was a problem hiding this comment.
When generating the string for a module, the ; are inserted after every statement, so Declaration/Assign shouldn't generate the semicolon to be consistent, or this semicolon should be removed
| @@ -259,7 +258,7 @@ std::string Always::toString() { | |||
| for (auto statement : body) { | |||
| always_str += | |||
| variant_to_string<BehavioralStatement *, Declaration *>(statement) + | |||
| ";\n"; | |||
| "\n"; | |||
There was a problem hiding this comment.
Again, the logic for generating the body of an always block is inconsistent with the logic for a module definition, we should use the same pattern for both (either everyone generates their own semicolon, or the enclosing construct adds the semicolons)
There was a problem hiding this comment.
Changing Always to be Structural makes sense, although Structural might not be the best name. Really what we want are a class of statements that are legal to put inside a module definition body (that is, behavioral statements must be inside an always block, whereas other statements must be inside a module definition). I feel like ModuleDefinitionStatement would be the most explicit/correct name, but that seems verbose.
Regardless, we can leave it as structural statement for now, but there's inconsistencies with how statements/semicolons are handled that should be resolved before merging.
The always block itself is a structural statement (in my opinion). I do not think we need to treat it differently than other structural statements. This also simplifies the AST slightly.