Skip to content

[refactor] Move Statement semantic routines to a new file and reduce imports#5730

Merged
WalterBright merged 1 commit intodlang:masterfrom
yebblies:ssemantic
Jun 6, 2016
Merged

[refactor] Move Statement semantic routines to a new file and reduce imports#5730
WalterBright merged 1 commit intodlang:masterfrom
yebblies:ssemantic

Conversation

@yebblies
Copy link
Contributor

@yebblies yebblies commented May 6, 2016

It's not especially clear from the diff, but this removes 14 imports from statement.d, and the new dsemantic.d is only imported by func.d. When complete the semantic analysis will be isolated from the rest of the compiler.

@yebblies yebblies force-pushed the ssemantic branch 3 times, most recently from 8339802 to e2fb57f Compare May 6, 2016 16:51
@9rnsr
Copy link
Contributor

9rnsr commented May 7, 2016

I cannot understand why the part of statement.d needs to go another module. If dsemantic.d needs to contain the code for "semantic analysis" in front-end, 90-% of front end code will have to be squashed into that file. It's terrible.

@yebblies
Copy link
Contributor Author

yebblies commented May 7, 2016

Yes, it will need to go into more than one file. The layout is yet to be determined. Do you have any suggestions?

It needs to be moved out to break the import cycle - the parser needs the ast nodes only, but currently the ast nodes include semantic which imports the whole compiler.

The goal is to allow using the parser as a library without importing the full semantic anslysis code and everything it imports.

@Geod24
Copy link
Member

Geod24 commented May 7, 2016

Yes, it will need to go into more than one file. The layout is yet to be determined. Do you have any suggestions?

That's what I would suggest:

Separate DDMD code and the frontend (not just lexer / parser):

  • ddmd.frontend -> The DMD "frontend", as in, all the code necessary for command line parsing, file opening / expansions, etc...
  • ddmd.glue -> Self explanatory
  • ddmd.backend -> Self explanatory
  • dlang -> Or something similar, would contain everything necessary to run semantic analysis on some code (that would be "libddmd").

Then I would aim for one module per AST node. Modules that share some stuff in common can live under the same package, i.e. dlang.declarations.aggregate.

By the way, two of the current circular references come from the isXXX pattern, and the visitor, e.g:

  • isClassDeclaration's return type depends on ClassDeclaration, but is declared in Dsymbol, a parent class. Likewise for any other declaration.
  • Dsymbol.accept depends on Visitor, which then depends on all AST node.

@yebblies
Copy link
Contributor Author

yebblies commented May 7, 2016

What you're calling the frontend is what I call the driver, and dlang is the frontend. I don't see any point in overcomplicating it by trying to move ast classes to separate files, this isn't java. I have no problem with the AST classes, Visitor, and the parser importing all ast classes and don't see any point in trying to change it.

Specifically, I meant that with the semantic routines moved from the AST classes, it will probably be nicer to have eg type semantic and expression semantic in different modules, just because of the size. How exactly we organize these modules is the open question.

@yebblies yebblies force-pushed the ssemantic branch 3 times, most recently from 5ad489f to d97a48a Compare May 7, 2016 19:13
@Geod24
Copy link
Member

Geod24 commented May 7, 2016

What you're calling the frontend is what I call the driver, and dlang is the frontend.

Yah, "driver" is definitely a better wording.

I don't see any point in overcomplicating it by trying to move ast classes to separate files, this isn't java. I have no problem with the AST classes, Visitor, and the parser importing all ast classes and don't see any point in trying to change it.

My suggestion didn't involve moving all of semantic to its own module. I think taking all of the complexity out of it's class context and putting it into a separate file harm readability more than it helps.

@yebblies
Copy link
Contributor Author

yebblies commented May 8, 2016

How do you propose using the parser as a library without importing all of semantic? This is not about trying to improve readability, it's making the parser usable as a library.

@Geod24
Copy link
Member

Geod24 commented May 8, 2016

How do you propose using the parser as a library without importing all of semantic? This is not about trying to improve readability, it's making the parser usable as a library.

What's the point of having a parsing only library ? There's already a good one out there, and if it decrease readability that much, its just not worth it.

@yebblies
Copy link
Contributor Author

yebblies commented May 8, 2016

Having other libraries available does not make dmd as a library less desirable. I disagree that this will hurt maintainability.

@PetarKirov
Copy link
Member

Making the compiler usable as a library should be top priority. I don't it will harm readability in any way. And even if it does temporary, the refactoring that will come later will most certainly make up for it. Nice initiative @yebblies 👍

@9rnsr
Copy link
Contributor

9rnsr commented May 9, 2016

@yebblies How about to add factory functions for each of AST nodes, and then parser will use them?

@yebblies
Copy link
Contributor Author

yebblies commented May 9, 2016

How about to add factory functions for each of AST nodes, and then parser will use them?

Yes, Walter and I talked about doing this and I agree it's useful. But it doesn't solve the problem that this PR does, so both are required.

@yebblies yebblies force-pushed the ssemantic branch 3 times, most recently from b884fea to 6c26b83 Compare May 15, 2016 15:54
@yebblies
Copy link
Contributor Author

@WalterBright Ping

@yebblies
Copy link
Contributor Author

@WalterBright @andralex If nobody has a better idea we should move forward with this. Otherwise the dependency cycles are not going to be broken and compiler-as-a-library is not going to make progress.

@dnadlinger
Copy link
Contributor

dnadlinger commented May 19, 2016

Could you add a brief module-level ddoc summary to ddmd.ssemantic (which, I assume, stands for _s_tatement semantic)? Looks good to me otherwise.

@yebblies
Copy link
Contributor Author

That sounds reasonable. It's easy enough to go through and document the public methods too now that they're all together.

which, I assume, stands for statement semantic

Yeah. I'm not exactly in love with the name. Maybe something like ddmd.semantic.statement would be better.

@mathias-lang-sociomantic
Copy link
Contributor

I hope we can later break it further down, but this sounds like a good first step.

@WalterBright
Copy link
Member

@yebblies has made a good case for it. My only objection is the name. How about statementsem? or stmtsemantic? Neither is that great, but better than ssemantic. I also don't want to introduce a subdirectory, which would happen if statement.semantic were used.

Change the name, and I'll automerge it.

@PetarKirov
Copy link
Member

@WalterBright are you in general against use of packages in dmd, or just against this particular instance? I'm asking because (at least IMO) improving the code organization via packages would make the codebase easier to understand, because for a newcomer it's definitely hard to know what's the purpose of every module.
(I'm just asking to know your opinion, I don't want to argue about this particular case.)

@yebblies
Copy link
Contributor Author

yebblies commented Jun 6, 2016

@WalterBright Changed the name to ddmd.statementsem and rebased. Ready to go.

@WalterBright
Copy link
Member

WalterBright commented Jun 6, 2016

@ZombineDev against. It's for the simple reason that I use grep a lot when dealing with code (and a bunch of other tools that don't deal well with subdirectories), and distributing files over various directories makes that unhelpful.

@WalterBright WalterBright merged commit c3fef6a into dlang:master Jun 6, 2016
@Geod24
Copy link
Member

Geod24 commented Jun 6, 2016

@ZombineDev against. It's for the simple reason that I use grep a lot when dealing with code (and a bunch of other tools that don't deal well with subdirectories), and distributing files over various directories makes that unhelpful.

How so ?
grep has the -R switch which allows you to search recursively, e.g. grep -nR "cast(CommaExp)" src. That makes searching an entire project quite easy.

Also, since we're talking about packages, the current file layout in DMD is not separate-compilation friendly...

@WalterBright
Copy link
Member

Oh, I knew someone would bring up the -R switch. Frankly, it's annoying. And I use other simple tools that don't deal naturally with subdirectories. Having the backend in a separate directory works well, because I almost never need to deal with both at the same time. But the front end, I run tools over the source all the time.

@mathias-lang-sociomantic
Copy link
Contributor

Oh, I knew someone would bring up the -R switch. Frankly, it's annoying. And I use other simple tools that don't deal naturally with subdirectories.

Your comment just peaked my curiosity :)
Alternatively to -R, If your tools support a list of file, an alias of find can easily do the trick.

Having the backend in a separate directory works well, because I almost never need to deal with both at the same time. But the front end, I run tools over the source all the time.

Having the backend in its own directory is great, would you be opposed to doing the same separation for the other parts of DMD (driver, frontend, glue) ?

This is a recurrent request of mine because for some time I've been playing around the idea of writing a bot that would use the frontend, and I now have something that kinda works.

Since the frontend is currently far from being a library, you can guess I had a lot of issues fun.
For example, the fact that the frontend is currently importing DMD specific part.
A cleaner (and flat) directory structure would be a step towards libddmd without compromising your ability to work on the frontend.
Would you be open to continue this discussion in a P.R. ?

@WalterBright
Copy link
Member

would you be opposed to doing the same separation for the other parts of DMD (driver, frontend, glue) ?

Yes.

A cleaner (and flat) directory structure would be a step towards libddmd without compromising your ability to work on the frontend.

You want subdirectories with a flat directory structure? That doesn't make sense.

@WalterBright
Copy link
Member

@mathias-lang-sociomantic , did you know that @yebblies is also working on making the front into a library? That's the point of this PR.

@yebblies yebblies deleted the ssemantic branch June 6, 2016 23:50
@Geod24
Copy link
Member

Geod24 commented Jun 8, 2016

@mathias-lang-sociomantic , did you know that @yebblies is also working on making the front into a library? That's the point of this PR.

Yes, I was pleased to learn that at DConf.

You want subdirectories with a flat directory structure? That doesn't make sense.

The suggestion was one directory for the driver, one for the frontend, one for the glue layer, and one from the backend. So you can run tools on the frontend source, but there's a clearer separation between the different part. E.g. src/dmd/driver/, src/dmd/frontend/, src/dmd/frontend/, src/dmd/backend/.

@RazvanN7
Copy link
Contributor

@yebblies have you made any measurements on how this affects compilation speed? I want to apply your visitor tactic on separating semantic for all ast nodes in the compiler and I am a bit worried of the overhead incurred for 2 virtual function calls instead of 1 (+ the instantiation of the semantic visitor class everytime you call the statement semantic routine).

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.

8 participants