Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Implement templated version of _d_newThrowable #3661

Merged
merged 4 commits into from
Jan 10, 2022

Conversation

teodutu
Copy link
Member

@teodutu teodutu commented Jan 4, 2022

This adds a templated version of _d_newThrowable to be used for lowering expressions like throw new E("something"); in the compiler's frontend.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @teodutu! 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 + druntime#3661"

@adamdruppe
Copy link
Contributor

Is your initial goal just to get it working, then go back later to redo the implementation?

@teodutu
Copy link
Member Author

teodutu commented Jan 4, 2022

Is your initial goal just to get it working, then go back later to redo the implementation?

To a certain extent, yes, but if there's something that needs improving, I'll try to do it now.

src/core/lifetime.d Outdated Show resolved Hide resolved
src/core/lifetime.d Outdated Show resolved Hide resolved
@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 6, 2022

@adamdruppe if you have any suggestions, feel free to comment. If there isn't anything blocking us from applying them, we will most certainly will. We want to have the best possible version of the code.

@adamdruppe
Copy link
Contributor

The way I'd actually do it is

  1. Extract the needed compile-time info in the template to remove the references to TypeInfo. traits should be able to do it all, except maybe a ctfe switch for the initializer like MoonlightSentinel said.

  2. Forward that information to a non-template function that actually has the implementation.

  3. Do whatever type casts are needed at the boundaries of the template.

This kind of type erasure keeps the template function itself small to avoid code bloat inside druntime by duplicating the generation of the implementation over and over again, while still taking advantage of the template to remove extra RTTI dependencies.

Since this is a small function anyway it might not matter much but still this is how I'd attempt all the template transitions.

But also this is an impl detail that can be done in a second pass.

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work labels Jan 6, 2022
Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
…rowable`

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@dlang-bot dlang-bot removed Needs Work Needs Rebase needs a `git rebase` performed labels Jan 6, 2022
@teodutu
Copy link
Member Author

teodutu commented Jan 6, 2022

1. Extract the needed compile-time info in the template to remove the references to TypeInfo. traits should be able to do it all, except maybe a ctfe switch for the initializer like MoonlightSentinel said.

2. Forward that information to a non-template function that actually has the implementation.

I think I can template-ise the hook right away, without resorting to this. I believe I've already done so by following @MoonlightSentinel's advice and using __traits(initSymbol, T).

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 7, 2022

@teodutu ping me when this is ready.

@teodutu
Copy link
Member Author

teodutu commented Jan 7, 2022

@RazvanN7 I'd say it is. It no longer uses TypeInfo_Class at all.

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.

Looks goot to me. cc @adamdruppe @MoonlightSentinel

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 7, 2022
@adamdruppe
Copy link
Contributor

I think this is good as it is now. The forwarding pattern is useful in other cases but here I don't think it would be a significant difference either way, so we can keep it simple and come back to tweak the implementation later if it proves to be a problem.

@RazvanN7 RazvanN7 added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Jan 10, 2022
@dlang-bot dlang-bot merged commit 786bc9e into dlang:master Jan 10, 2022
teodutu added a commit to teodutu/druntime that referenced this pull request Jun 8, 2022
It was replaced by a template hook:
- dlang#3661
- dlang/dmd#13494

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
@teodutu teodutu deleted the template-_d_newThrwoable branch June 15, 2022 21:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants