Integrate ASTBase visitor with the compiler visitor

[RazvanN7]

In order to create a transitive visitor for the compiler like the one in the parser library, the existing visitor had to be tweaked a bit.

[dlang-bot]

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[RazvanN7]

Does anyone know why the autotester is failing while circleci compiles successfully? Compiling on my machine works perfectly.

[andralex]

Note: much of this is a workaround to https://issues.dlang.org/show_bug.cgi?id=18035

[stefan-koch-sociomantic]

why did asttypename.d get removed from the build ?
It is a pretty important debugging utility!

[jacob-carlborg]

@stefan-koch-sociomantic it was because it caused the auto-tester to fail [1]. It think because it's running an older version of DMD. It also needs to be compatible with GDC.

[1] https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=2929293&isPull=true

[ibuclaw]

Speaking of compatibility with GDC. Did no one here think that renaming class Visitor would break the C++ API?

You've got to stop doing this.

[andralex]

@ibuclaw sorry! The best way to ensure that doesn't happen anymore is to have some unittests to that effect. @RazvanN7 can you please investigate using inheritance of class Visitor instead of aliasing? Thx!

[stefan-koch-sociomantic]

The reason asttypename.d caused the build to fail is because of the change in visibility rules; and this indicates problems for everyone wanting to use the generic visitor. When the visibility/import rules get stricter again.

[ibuclaw]

@andralex - Perhaps a test (written in C++) that links to and uses the front-end would suffice?

Covering all cases (virtual functions, field accessors) would be no small thing, particularly if you are "mocking" a compiler.

[jacob-carlborg]

@andralex @RazvanN7 what about a tool that generates a C/C++ header file from a D module? If possible, built on the Dub package.

[ibuclaw]

Generating the headers is only half the problem. Notifying downstream of changes is just as important.

[andralex]

Perhaps a test (written in C++) that links to and uses the front-end would suffice?

Good idea, @RazvanN7 what do you think?

[RazvanN7]

@ibuclaw

You've got to stop doing this.

It's not a simple class rename, but rather a design modification.

Notifying downstream of changes is just as important.

How do you suggest the downtstream notification should be done?

Good idea, @RazvanN7 what do you think?

@andralex I think that creating such a test requires a big effort for a small gain: in order to be sure that the test is relevant you would have to access all the fields and methods of the compiler. On the other hand, it would be much simpler to create a tool that automatically creates the C++ header. For 90% of the cases the implementation is trivial. Once we have the header generation tool, the integration should not pose any more problems.

Now, specifically for this PR, I suggest we modify the visitor.h file to reflect the changes as I think that the current design of the visitors is suited for the future compiler library. What this PR does is to move the visiting methods for AST nodes that are generated at parse time into a templated visitor (ParseTimeVisitor) and have a generic visitor inherit the ParseTimeVisitor and implement the visiting methods for AST nodes generated at semantic time. The alias Visitor = GenericVisitor!ASTCodegen is used in order to not modify the accept methods of all AST nodes. Now I am no expert in D-C++ linking,
but since the name Visitor aliases a visitor which is instantiated with the code generation AST family shouldn't the Visitor class defined in visitor.h be the same as the aliased one once the ParseTimeVisitor is appropriately added in the header file?

[wilzbach]

On the other hand, it would be much simpler to create a tool that automatically creates the C++ header. For 90% of the cases the implementation is trivial. Once we have the header generation tool, the integration should not pose any more problems.

Yes, I agree - creating the header generation tool is a good way to move forward. This will also be useful for other projects who would like to have a tight C++ integration ;-)

Notifying downstream of changes is just as important.

@RazvanN7: for a start, you could either add a changelog entry for bigger changes or CC important people like @ibuclaw

@andralex @RazvanN7 what about a tool that generates a C/C++ header file from a D module? If possible, built on the Dub package.

This should help: #7425

[ibuclaw]

It's not a simple class rename, but rather a design modification.

And where is the C++ interface for it? As a user of the Visitor class for all code generation passes, has this been tested that it doesn't break existing downstream code?

[RazvanN7]

Attempted fix : #7438