Translate _d_delstruct to template by teodutu · Pull Request #13398 · dlang/dmd

teodutu · 2021-12-07T16:30:11Z

This PR translates the lowering of expressions like delete p to a template version of _d_delstruct that no longer uses the second TypeInfo_Struct parameter. The PR where this new template hook was implemented is this: dlang/druntime#3639

dlang-bot · 2021-12-07T16:30:15Z

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 + dmd#13398"

src/dmd/errors.d

src/dmd/expressionsem.d

RazvanN7 · 2021-12-08T08:01:22Z

src/dmd/e2ir.d

+                 * expression. This is how e2ir.d gets here.
+                 * Since the lowering has already been made, the DeleteExp can
+                 * be ignored here.
+                 */


How is this going to affect the traceGC hooks? If you look below, there is a call to toTraceGC which basically calls a wrapper of the hook if the gc_profile command line flag is passed. It seems that in this case, the wrapper call is ignored.

RazvanN7

Looks good to me

RazvanN7 · 2021-12-15T09:09:20Z

src/dmd/expressionsem.d

+                 * `_d_delstruct` in order to keep checking for dtor errors as
+                 * well as generate code for the call to `_d_delstruct`.
+                 */
+                e = Expression.combine(exp, e);


Why do we still need the original expression and pass it all the way to e2ir? Can't we simply check here if the dtor is callable in the current context and output an error if not?

I tried to do this, but certain errors are output from different contexts, which are difficult to replicate here.

Here's an example: this snippet [1] may be output an error if the dtor throws. In order to verify this, canThrow is called here [2]. _d_delstruct(s) is nothrow so that it's not mentioned in errors such as this one [3].

I agree that returning both expressions is inelegant, but I don't see a way to avoid the situation above without duplicating the logic from semantic3.d in expressionsem.d.

[1]

dmd/src/dmd/semantic3.d

Lines 747 to 750 in 93108bb

// Check for errors related to 'nothrow'.

const blockexit = funcdecl.fbody.blockExit(funcdecl, f.isnothrow);

if (f.isnothrow && blockexit & BE.throw_)

error(funcdecl.loc, "`nothrow` %s `%s` may throw", funcdecl.kind(), funcdecl.toPrettyChars());

[2]

dmd/src/dmd/blockexit.d

Lines 113 to 114 in 93108bb

if (canThrow(s.exp, func, mustNotThrow))

result |= BE.throw_;

[3]

dmd/src/dmd/canthrow.d

Lines 58 to 64 in 93108bb

if (mustNotThrow)

{

e.error("%s `%s` is not `nothrow`",

f.kind(), f.toPrettyChars());

e.checkOverridenDtor(null, f, dd => dd.type.toTypeFunction().isnothrow, "not nothrow");

}

src/dmd/frontend.h

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Such calls are returned from expressionsem.d together with their corresponding DeleteExp, as part of a CommaExp. Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Signed-off-by: teo <teodor.dutu@gmail.com>

RazvanN7

Does not need to recreate the DeleteExp.

RazvanN7 · 2021-12-16T16:54:50Z

src/dmd/canthrow.d

+                // In expressionsem.d `delete s` was lowered to `_d_delstruct(s)`.
+                // The following code rewrites it back to the original expresion
+                // in order to properly output `nothrow` errors.
+                auto de = new DeleteExp(ce.loc, (*ce.arguments)[0], false);


Creating a DeleteExp is superfluous here. This is wasteful because allocation is costly. You can simply check if the dtor of of the aggregate type (if it exists) throws: this is essentially what the DeleteExp visiting method does.

I was trying to not duplicate the code in visit(DeleteExp), but I see your point.

RazvanN7 · 2021-12-16T16:56:33Z

src/dmd/dinterpret.d

+                // interpret that expression.
+                assert(e.arguments.dim == 1);
+
+                auto de = new DeleteExp(e.loc, (*e.arguments)[0], false);


RazvanN7 · 2021-12-16T16:56:41Z

src/dmd/nogc.d

+            // In expressionsem.d `delete s` was lowered to `_d_delstruct(s)`.
+            // The following code rewrites it back to the original expresion in
+            // order to properly output `nogc` errors.
+            auto de = new DeleteExp(e.loc, (*e.arguments)[0], false);


…`nogc` and `nothrow` Signed-off-by: teo <teodor.dutu@gmail.com>

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

RazvanN7 · 2021-12-17T07:13:13Z

Great work, @teodutu ! Thanks for this. Let's wait a bit to see if someone else has anything to comment and then we'll merge.

These hooks have been converted to templates and their lowerings changed to use those templates: - dlang/dmd#13116 - dlang/dmd#13495 - dlang/dmd#13398 Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

teodutu requested a review from ibuclaw as a code owner December 7, 2021 16:30

RazvanN7 reviewed Dec 8, 2021

View reviewed changes

teodutu force-pushed the convert-_d_delstruct-hook branch from 3d0c446 to 94803a6 Compare December 8, 2021 09:14

dlang-bot added Review:Needs Rebase Review:Needs Work labels Dec 9, 2021

teodutu mentioned this pull request Dec 9, 2021

Add template _d_delstructTrace dlang/druntime#3644

Merged

teodutu force-pushed the convert-_d_delstruct-hook branch from 94803a6 to 15a898b Compare December 9, 2021 19:47

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Dec 9, 2021

teodutu force-pushed the convert-_d_delstruct-hook branch 3 times, most recently from 683ce63 to 32a4d4f Compare December 14, 2021 17:46

RazvanN7 approved these changes Dec 15, 2021

View reviewed changes

RazvanN7 reviewed Dec 15, 2021

View reviewed changes

teodutu force-pushed the convert-_d_delstruct-hook branch from 32a4d4f to 9b25792 Compare December 16, 2021 16:47

RazvanN7 reviewed Dec 16, 2021

View reviewed changes

src/dmd/frontend.h Show resolved Hide resolved

teodutu added 8 commits December 16, 2021 18:53

Translate _d_delstruct to template

45bd487

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Suppress errors from _d_delstruct

7ebfe0c

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

_d_delstruct: Return both DeleteExp and lowering from expressionsem.d

c005bf0

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

_d_delstruct: Ignore hook during CTFE

49faa32

Such calls are returned from expressionsem.d together with their corresponding DeleteExp, as part of a CommaExp. Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

_d_delstruct: Clarify combining the lowering and the DeleteExp

076c125

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Use _d_delstructTrace when compiling with -gc_profile

20c6a96

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

Gag errors for _d_delstruct when lowering to it

67b874e

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

expressionsem.d: Only return the lowering to _d_delstruct

45e22ef

Signed-off-by: teo <teodor.dutu@gmail.com>

teodutu force-pushed the convert-_d_delstruct-hook branch from 9b25792 to 45e22ef Compare December 16, 2021 16:54

RazvanN7 requested changes Dec 16, 2021

View reviewed changes

teodutu added 2 commits December 16, 2021 21:10

Do not create additional DeleteExps when checking _d_delstruct for …

3c673fc

…`nogc` and `nothrow` Signed-off-by: teo <teodor.dutu@gmail.com>

End compilation if a delete reaches e2ir.d instead of _d_delstruct

0dd63f2

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

RazvanN7 approved these changes Dec 17, 2021

View reviewed changes

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Dec 17, 2021

RazvanN7 merged commit 5e757c8 into dlang:master Dec 20, 2021

teodutu deleted the convert-_d_delstruct-hook branch December 20, 2021 16:22

teodutu mentioned this pull request May 26, 2022

Remove previously templated hooks dlang/druntime#3826

Merged

	// Check for errors related to 'nothrow'.
	const blockexit = funcdecl.fbody.blockExit(funcdecl, f.isnothrow);
	if (f.isnothrow && blockexit & BE.throw_)
	error(funcdecl.loc, "`nothrow` %s `%s` may throw", funcdecl.kind(), funcdecl.toPrettyChars());

	if (canThrow(s.exp, func, mustNotThrow))
	result \|= BE.throw_;

	if (mustNotThrow)
	{
	e.error("%s `%s` is not `nothrow`",
	f.kind(), f.toPrettyChars());

	e.checkOverridenDtor(null, f, dd => dd.type.toTypeFunction().isnothrow, "not nothrow");
	}

Uh oh!

Conversation

teodutu commented Dec 7, 2021

Uh oh!

dlang-bot commented Dec 7, 2021

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RazvanN7 Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

teodutu Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

teodutu Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants