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

Support nested classes #4178

Open
tudortimi opened this issue May 7, 2023 · 7 comments
Open

Support nested classes #4178

tudortimi opened this issue May 7, 2023 · 7 comments
Assignees
Labels
status: assigned Issue is assigned to someone to work on type: feature-IEEE Request to add new feature, described in IEEE 1800

Comments

@tudortimi
Copy link
Contributor

Is support for nested classes something reasonably easy to add (e.g. for a beginner)?

@tudortimi tudortimi added the new New issue not seen by maintainers label May 7, 2023
@tudortimi
Copy link
Contributor Author

I generally use nested classes a lot to avoid "polluting" scopes. Sometimes, they also make more sense, because the nesting class creates a nice sub-scope.

@wsnyder wsnyder changed the title Add support for nested classes Support nested classes May 7, 2023
@wsnyder wsnyder added type: feature-IEEE Request to add new feature, described in IEEE 1800 and removed new New issue not seen by maintainers labels May 7, 2023
@wsnyder
Copy link
Member

wsnyder commented May 7, 2023

I presume we will flatten out the classes, that is a nested class will become non-nested in the C++, otherwise things will get "interesting". So, the first part is to support elaboration of any nest, then supporting nesting where there are class parameters (inside/outside/both). The hard part will be stomping out the bugs that result.

Likely a few week project, if you'd like to start. We could always e.g. support nesting only when there are no parameters, then improve from there.

@wsnyder wsnyder added the status: ready Issue is ready for someone to fix; then goes to 'status: assigned' label May 7, 2023
@tudortimi
Copy link
Contributor Author

Sure, how would one start with just the "no parameters" part?

@wsnyder
Copy link
Member

wsnyder commented May 7, 2023

First, make a bunch of tests that try some complicated things and make sure they pass on other simulators. I'd suggest separate out non-parameter and parameter tests. Some of the complicated things to try in both the inner/outer:

parameters
extends
function new
member variables
member functions
static methods
virtual methods
accessing outer static/virtual/member functions from inner

That is, similar to what much of what the current tests check, but in the inner context.

This can be a pull we merge without any other support ready, the tests would just check Verilator will error out.

Then start on support.

Delete the current error, run with --debug --debugi-V3LinkDot to see how the .tree files look, and the .txt file that describes the symbol table. Get through V3LinkDot stage (this will be probably 70% of the work).

V3Class (I think) needs to learn how to pull apart the nested classes to flatten them. Past that point (once flat) in the processing pipeline, it should be just like there's no nested classes to the internals, so hopefully only a few changes may result.

@tudortimi
Copy link
Contributor Author

Understood, I'll start thinking of tests.

@tudortimi
Copy link
Contributor Author

tudortimi commented Sep 18, 2023

Can you assign this to me so it shows up in my dashboards?

@wsnyder wsnyder added status: assigned Issue is assigned to someone to work on and removed status: ready Issue is ready for someone to fix; then goes to 'status: assigned' labels Sep 18, 2023
@wsnyder
Copy link
Member

wsnyder commented Sep 18, 2023

Great, we welcome work on this.

In addition to suggestions above, you should audit all "visit (AstClass" to see if what they do makes sense if they are recursed upon due to a class-in-class. Generally they should be using "VL_RESTORER" so they handle it, but likely there's some escapes as this hasn't been tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: assigned Issue is assigned to someone to work on type: feature-IEEE Request to add new feature, described in IEEE 1800
Projects
None yet
Development

No branches or pull requests

2 participants