Skip to content

Comments

Add dmd as a lib callbacks for statement semantic#11092

Merged
dlang-bot merged 8 commits intodlang:masterfrom
cristiancreteanu:APIchange
Jun 7, 2020
Merged

Add dmd as a lib callbacks for statement semantic#11092
dlang-bot merged 8 commits intodlang:masterfrom
cristiancreteanu:APIchange

Conversation

@cristiancreteanu
Copy link
Contributor

@cristiancreteanu cristiancreteanu commented May 2, 2020

While trying to change the implementation of DCD to use DMD as a library, I have come across the need to retrieve the Scope of a Statement (namely the Statement at the location in the source file where the cursor is) without having too much of an impact on DMD's performance, but I couldn't find an easy way to do so by only using a visitor on the AST, as not all nodes had a Scope field or, if they did, it was null (in particular the _scope field of Dsymbol, which was null, except for field declarations). I took Basile's suggestions on the forum (https://forum.dlang.org/post/pinxxdclrvdnidpnxixr@forum.dlang.org).

With the current approach, however, I will not get results if the node whose Scope I am trying to find is a field declaration inside a class/struct/enum. To solve this problem, I could either use the same approach that I used for Statement and add delegates for Dsymbol in dsymbolsem or I could implement a custom visitor which traverses the AST and reaches the required node (which will be a Dsymbol whose _scope is set). What do you think is the best solution in this case?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @cristiancreteanu! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#11092"

@ibuclaw
Copy link
Member

ibuclaw commented May 3, 2020

Would make more sense maybe if these could be fields inside the Compiler structure.

@rainers
Copy link
Member

rainers commented May 3, 2020

I have added a ScopeDsymbol to FuncDeclaration and ScopeStatement for a similar purpose, see rainers@bc9b1c7

The visitor to find the AST node for the caret is here: https://github.com/dlang/visuald/blob/master/vdc/dmdserver/semvisitor.d#L625 but there is also some additional machinery in place, e.g. remembering the original AST when it is modified during semantic analysis.

@cristiancreteanu
Copy link
Contributor Author

Would make more sense maybe if these could be fields inside the Compiler structure.

Now that you mention this, maybe keeping them on a global level isn't such a good idea, but I would rather add them as static fields inside the Statement class, as I am thinking that, since they are used for the semantic analysis of Statement instances, that's where they would probably be suited to be. But, based on your experience, could you please tell me why it would be better to add them in the Compiler structure?

@cristiancreteanu
Copy link
Contributor Author

I have added a ScopeDsymbol to FuncDeclaration and ScopeStatement for a similar purpose, see rainers@bc9b1c7

The visitor to find the AST node for the caret is here: https://github.com/dlang/visuald/blob/master/vdc/dmdserver/semvisitor.d#L625 but there is also some additional machinery in place, e.g. remembering the original AST when it is modified during semantic analysis.

Thanks! So I understand that VisualD uses your fork instead of the latest version of DMD. How come you haven't merged those changes to DMD? It would also solve my issue.

@rainers
Copy link
Member

rainers commented May 3, 2020

Thanks! So I understand that VisualD uses your fork instead of the latest version of DMD.

Yes.

How come you haven't merged those changes to DMD? It would also solve my issue.

Mostly to avoid endless discussions and fighting with the CI;-) Not all changes might be uncontroversial, and some probably need polishing.

#10846 was actually an attempt to start with creating small PRs, but that hit a road block because some tooling seems to depend on the current state, but it is outside the dlang repositories.

@ibuclaw
Copy link
Member

ibuclaw commented May 3, 2020

Would make more sense maybe if these could be fields inside the Compiler structure.

Now that you mention this, maybe keeping them on a global level isn't such a good idea, but I would rather add them as static fields inside the Statement class, as I am thinking that, since they are used for the semantic analysis of Statement instances, that's where they would probably be suited to be. But, based on your experience, could you please tell me why it would be better to add them in the Compiler structure?

There's already Compiler.onImport and Compiler.loadModule (which I want to get renamed to something like onModuleLoad) which both are similar callbacks from frontend to compiler driver. The dmd.compilur module is intended to be implemented independently by each glue frontend, so gdc and ldc can each implement their own actions if required.

@RazvanN7 RazvanN7 changed the title Added before & after functionality in statement semantic Add dmd as a lib callbacks for statement semantic May 16, 2020
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test and we are good to go.

Is anyone against this addition? This makes dmd as a library usable with DCD.

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also hide the other changes behind version (Callback_API), they are unused when compiling without this version.

Otherwise LGTM

@cristiancreteanu
Copy link
Contributor Author

I separated the creation of a single module so that I could use the function as part of the compiler's interface, as it can be seen in the test. It was also possible to use the already existent createModules, but it was a bit less straightfoward imo. Having the use case in the test in mind, I realised that if I wanted to work with a single module, first I'd have had to create an array of module file names with only one element, then call createModules and get the first element in the array of modules. This change also separates the creation of a module from the additional logic inside createModules and makes clear which part of the function handles the actual instantiation of a single module. Let me know if I should be working with the existing interface of mars.d instead and revert the change.

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal changes are fine by me. Cannot really comment on it's usefullness for DMD as a library though.

@RazvanN7 RazvanN7 dismissed MoonlightSentinel’s stale review June 7, 2020 12:33

The changes were addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants