[WIP] Lower newExp for scalar types

[edi33416]

Lower calls to new expression to druntime's _d_newitemT templated implementation that uses compile time information about the type instead of TypeInfo.

[dlang-bot]

Thanks for your pull request and interest in making D better, @edi33416! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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 fetch digger
dub run digger -- build "master + dmd#9433"

[edi33416]

@andralex @RazvanN7 @WalterBright

[WalterBright]

newExpLowering = newExpLowering.expressionSemantic(sc);

[WalterBright]

You're also calling expressionSemantic() twice on the same expression.

[jacob-carlborg]

Even this works:

newExpLowering = new ConstructExp(newExpLowering.loc, newExpLowering, e.addressOf()).expressionSemantic(sc);

But it might be a bit too long.

[edi33416]

I addressed the feedback. This is still a WIP, as there are some tests failing in druntime.
I'm getting the following errors

src/object.d(63): Error: static variable typeid(int) cannot be read at compile time                                                                                 
src/object.d(88):        called from here: ___d_newitemU()                                                                                                          
src/core/lifetime.d(1621):        called from here: ___d_newitemT()                                                                                                 
src/core/internal/traits.d(334):        called from here: (*function () 

[...]

src/core/internal/traits.d(334):        called from here: (*function () => true)()
src/core/internal/traits.d(334):        while evaluating: static assert((*function () => true)())
src/core/lifetime.d(1606): Error: template instance `core.internal.traits.assertCTFEable!(function ()

How was this previously CTFEable and what changed now?

Another issue that I'm having is that I can't call a non-templated function from ___d_newitemT. The linker will fail saying it doesn't find the function. Any ideas why this might happen?

Edit: @WalterBright @jacob-carlborg could you please help me with the above?

[PetarKirov]

No need for triple underscore prefix and T suffix:

Suggested change

	{ "___d_newitemT" },
	{ "__newitem" },

[rainers]

I also consider double underscore to be wrong usage for regular identifiers in dmd, as these are reserved by the language specification.

There are a couple of places where an identifier behaves slightly different if it starts with __, e.g. https://github.com/dlang/dmd/blob/master/src/dmd/traits.d#L1392.

The mago debugger can also hide variables that start with a double underscore as these are usually compiler generated temporaries.

[PetarKirov]

I agree with @rainers - the function are extern (D) and templates non the less, so we don't need to resort to legacy C-style naming practices. I think for the time being, we should at least use a name consistent with __switch_error and __equals, like __new_item or more precisely __new_scalar, though newHeapObject (or something along those lines) would be better, considering that we probably want to lower all new expressions to it.

[JinShil]

I think the problem is these symbols are in object.d which is automatically imported by all .d source files. This could cause a naming conflict in user code. If we could fully qualify the symbol (e.g. object.switchError instead of just switchError) in the lowering, we wouldn't need the double underscore prefix. I don't know how to do that. @WalterBright, do you?

[rainers]

Although the use of double underscores is probably unnecessary in object.d, too, I wasn't actually referring to their use in lowerings, but their use in dmd as Id.__newitem. That way a reserved identifier used in lowerings appears in regular code aswell.

[wilzbach]

If we could fully qualify the symbol (e.g. object.switchError instead of just switchError) in the lowering,

Sure we can and have done that even in the past ;-)
It looks like this:

dmd/src/dmd/expressionsem.d

Lines 5663 to 5666 in fd8eb47

	Expression __assertFail = new IdentifierExp(exp.loc, Id.empty);
	auto assertFail = new DotIdExp(loc, __assertFail, Id.object);
	auto dt = new DotTemplateInstanceExp(loc, assertFail, Id._d_assert_fail, tiargs);

[jacob-carlborg]

If we could fully qualify the symbol

That should be a requirement. I've already run into issues with a similar case where the fully qualified name wasn't used.

[jacob-carlborg]

Regarding the double underscore. Since these need to be public symbols, the double underscore could be used to indicate that these symbols should not be used directly, only by the compiler.

[JinShil]

Error: static variable typeid(int) cannot be read at compile time

I doubt the original runtime hook was ever called at compile-time. It's currently lowered in e2ir.d which is after all semantic and CTFE opportunity (if my understanding of the compiler is correct). By converting this runtime hook to a template, and lowering in the semantic phase of the compiler, you've now subject the runtime template to CTFE, which has its consequences.

The best course of action is to just remove typeid completely from the runtime template. As a hack, you could also add a check for sc.flags & SCOPE.ctfe, but that will earn low marks.

[jacob-carlborg]

Should use the fully qualified name when calling the function.

[dkorpel]

Superseded by #14664