Skip to content

Comments

[WIP] Translate _d_arraycatnTX to template#10064

Closed
Vild wants to merge 6 commits intodlang:masterfrom
Vild:ConvertArrayCatHook
Closed

[WIP] Translate _d_arraycatnTX to template#10064
Vild wants to merge 6 commits intodlang:masterfrom
Vild:ConvertArrayCatHook

Conversation

@Vild
Copy link
Contributor

@Vild Vild commented Jun 19, 2019

druntime PR: dlang/druntime#2648

These patches will lower a ~ a to (S[][2] __cat2 = [arr1, arr1];) , _d_arraycatnTX(cast(S[][])__cat2).

@Vild Vild changed the title Convert array cat hook [WIP] Convert array cat hook Jun 19, 2019
@JinShil
Copy link
Contributor

JinShil commented Jun 19, 2019

// This line will assert the compile:
//     dmd: dmd/backend/cod4.d:401: Assertion `cast(int)sz > 0' failed.
S[] arr3 = arr1 ~ arr1 ~ arr1;

@WalterBright Could you please lend your expertise to help us solve the backend assertion failure?

@JinShil
Copy link
Contributor

JinShil commented Jun 20, 2019

So I guess I need to mark my static-array as a scope type,, but I could not find how to do that.

I believe you should be able to do that with Parameter fparam; fparam.storageClass |= STC.scope_

Though I'm not sure how to do that when all you have is an array of Expressions. I'll look into it.

Edit: Gah, you can't do that. You have to do it at the function declaration site. I guess I don't understand. Please clarify.

@Vild Vild force-pushed the ConvertArrayCatHook branch from 16ef63e to 45e13d4 Compare June 25, 2019 16:59
@Vild Vild marked this pull request as ready for review June 30, 2019 23:43
@Vild Vild force-pushed the ConvertArrayCatHook branch from b6be734 to b0e932e Compare June 30, 2019 23:43
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 30, 2019

Thanks for your pull request and interest in making D better, @Vild! 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 fetch digger
dub run digger -- build "master + dmd#10064"

@Vild Vild changed the title [WIP] Convert array cat hook Convert array cat hook Jun 30, 2019
@Vild Vild changed the title Convert array cat hook Translate _d_arraycatnTX to template Jun 30, 2019
@Vild Vild force-pushed the ConvertArrayCatHook branch from b0e932e to bacbf8e Compare July 9, 2019 13:39
Vild added 6 commits July 22, 2019 03:11
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Without this the first argument turns into a `string`
src/dmd/mars.d(2174): Error: function
`dmd.mars.parseCommandLine.error(const(char)* format, const(char*) arg
= null)` is not callable using argument types `(string, const(char)*)`

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Expression ea = (*e.arguments)[0];

while (true)
Copy link
Member

Choose a reason for hiding this comment

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

You have used a simpler while(ea.isCastExp()) ea = ea.isCastExp().e1; elsewhere.

}

if (global.params.verbose)
message("interpret %s =>\n %s", e.toChars(), e1.toChars());
Copy link
Member

Choose a reason for hiding this comment

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

Do not use message for your debug output, use versioned printf instead.

exp.type = tb1next.arrayOf();
L2elem:
if (tb2.ty == Tarray || tb2.ty == Tsarray)
if (tb2.ty != Tarray && tb2.ty != Tsarray)
Copy link
Member

Choose a reason for hiding this comment

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

Why can you completely invert the condition here?

result = exp.optimize(WANTvalue);
return;
e = exp.optimize(WANTvalue);
goto Lrewrite;
Copy link
Member

Choose a reason for hiding this comment

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

Can you place the rewriting into a (local) function instead of having to use goto?

if (!flag)
{
error("unknown JSON field `-Xi=%s`, expected one of " ~ jsonFieldNames, p + 4);
error(cast(const(char)*)("unknown JSON field `-Xi=%s`, expected one of " ~ jsonFieldNames), p + 4);
Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked closely at the actual rewrite, but if it needs this additional cast (probably concatenation moved from CT to RT) it will break quite some code,

@Vild Vild changed the title Translate _d_arraycatnTX to template [WIP] Translate _d_arraycatnTX to template Jul 23, 2019
@dlang-bot dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Jul 23, 2019
@RazvanN7
Copy link
Contributor

Closing as @teodutu is taking care of this.

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

Labels

Review:Needs Rebase Review:Needs Work Review:stalled Review:WIP Work In Progress - not ready for review or pulling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants