Implement visitor for toExpression functions by RazvanN7 · Pull Request #7049 · dlang/dmd

RazvanN7 · 2017-07-31T12:18:12Z

Related PRs:

dlang-bot · 2017-07-31T12:18:12Z

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

UplinkCoder · 2017-07-31T19:57:57Z

You added a new file which now contains semantically unrelated code in close visual proximity.
And perf slow downs.

WalterBright · 2017-07-31T22:41:19Z

The toExpression() for type and toExpression() for Initializer should be named typeToExpression() and initializerToExpression() as they have nothing to do with each other (and hence should be distinguished by name rather than overloading). I recommend typeToExpression() going into initsem.d, and typeToExpression() into typesem.d.

RazvanN7 · 2017-08-01T11:13:50Z

I love it how the autotester passes all tests and the rest are red

RazvanN7 · 2017-08-01T11:40:38Z

@WalterBright done. Is it ok now?

RazvanN7 · 2017-08-01T11:51:37Z

I thought about aliasing the functions to toExpression, but it is faster to call initializerToExpression and typeToExpression directly.

andralex · 2017-08-01T14:53:04Z

Ran speed measurements in the same manner as #7031. Baseline (master):

make -j1 -f posix.mak > /dev/null  5.01s user 0.16s system 91% cpu 5.674 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.15s system 93% cpu 5.494 total
make -j1 -f posix.mak > /dev/null  5.12s user 0.06s system 93% cpu 5.550 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.09s system 93% cpu 5.477 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.14s system 93% cpu 5.504 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.13s system 93% cpu 5.521 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.14s system 93% cpu 5.516 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.16s system 93% cpu 5.483 total
make -j1 -f posix.mak > /dev/null  4.95s user 0.14s system 93% cpu 5.452 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.13s system 93% cpu 5.540 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.11s system 93% cpu 5.518 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.14s system 93% cpu 5.565 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.12s system 93% cpu 5.503 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.11s system 93% cpu 5.506 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.15s system 93% cpu 5.491 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.13s system 93% cpu 5.501 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.11s system 93% cpu 5.508 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.12s system 93% cpu 5.495 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.12s system 93% cpu 5.474 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.12s system 93% cpu 5.521 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.13s system 93% cpu 5.555 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.11s system 93% cpu 5.471 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.13s system 93% cpu 5.503 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.11s system 93% cpu 5.463 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.15s system 93% cpu 5.510 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.18s system 93% cpu 5.495 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.12s system 93% cpu 5.505 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.15s system 93% cpu 5.454 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.14s system 93% cpu 5.498 total
make -j1 -f posix.mak > /dev/null  4.95s user 0.12s system 93% cpu 5.446 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.17s system 93% cpu 5.529 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.16s system 93% cpu 5.507 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.15s system 93% cpu 5.492 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.14s system 93% cpu 5.532 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.511 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.18s system 93% cpu 5.485 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.14s system 93% cpu 5.492 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.12s system 93% cpu 5.501 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.09s system 93% cpu 5.513 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.14s system 93% cpu 5.562 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.16s system 93% cpu 5.554 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.12s system 93% cpu 5.528 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.11s system 93% cpu 5.477 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.18s system 93% cpu 5.532 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.13s system 93% cpu 5.536 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.11s system 93% cpu 5.476 total
make -j1 -f posix.mak > /dev/null  4.95s user 0.14s system 93% cpu 5.451 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.14s system 93% cpu 5.492 total
make -j1 -f posix.mak > /dev/null  5.07s user 0.14s system 93% cpu 5.589 total
make -j1 -f posix.mak > /dev/null  4.97s user 0.13s system 93% cpu 5.466 total

With this PR:

make -j1 -f posix.mak > /dev/null  5.17s user 0.14s system 93% cpu 5.679 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.10s system 93% cpu 5.505 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.14s system 93% cpu 5.492 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.15s system 93% cpu 5.453 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.18s system 93% cpu 5.551 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.16s system 93% cpu 5.511 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.14s system 93% cpu 5.477 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.515 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.12s system 93% cpu 5.510 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.15s system 93% cpu 5.493 total
make -j1 -f posix.mak > /dev/null  4.93s user 0.18s system 93% cpu 5.476 total
make -j1 -f posix.mak > /dev/null  5.05s user 0.12s system 93% cpu 5.537 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.10s system 93% cpu 5.496 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.11s system 93% cpu 5.509 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.12s system 93% cpu 5.486 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.18s system 93% cpu 5.585 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.13s system 93% cpu 5.487 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.15s system 93% cpu 5.482 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.12s system 93% cpu 5.456 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.14s system 93% cpu 5.506 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.15s system 93% cpu 5.472 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.14s system 93% cpu 5.500 total
make -j1 -f posix.mak > /dev/null  5.10s user 0.11s system 93% cpu 5.576 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.16s system 93% cpu 5.529 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.15s system 93% cpu 5.501 total
make -j1 -f posix.mak > /dev/null  4.99s user 0.10s system 93% cpu 5.465 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.13s system 92% cpu 5.557 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.13s system 93% cpu 5.513 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.16s system 93% cpu 5.464 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.11s system 93% cpu 5.423 total
make -j1 -f posix.mak > /dev/null  5.04s user 0.14s system 93% cpu 5.561 total
make -j1 -f posix.mak > /dev/null  5.01s user 0.12s system 93% cpu 5.497 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.13s system 93% cpu 5.559 total
make -j1 -f posix.mak > /dev/null  5.00s user 0.10s system 93% cpu 5.481 total
make -j1 -f posix.mak > /dev/null  5.07s user 0.12s system 93% cpu 5.559 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.14s system 93% cpu 5.579 total
make -j1 -f posix.mak > /dev/null  4.94s user 0.16s system 93% cpu 5.464 total
make -j1 -f posix.mak > /dev/null  5.06s user 0.13s system 93% cpu 5.556 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.16s system 93% cpu 5.494 total
make -j1 -f posix.mak > /dev/null  4.93s user 0.16s system 93% cpu 5.459 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.13s system 93% cpu 5.481 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.11s system 93% cpu 5.498 total
make -j1 -f posix.mak > /dev/null  5.03s user 0.14s system 93% cpu 5.547 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.16s system 93% cpu 5.486 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.12s system 93% cpu 5.477 total
make -j1 -f posix.mak > /dev/null  5.08s user 0.10s system 93% cpu 5.555 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.10s system 93% cpu 5.489 total
make -j1 -f posix.mak > /dev/null  4.96s user 0.11s system 93% cpu 5.428 total
make -j1 -f posix.mak > /dev/null  4.98s user 0.14s system 93% cpu 5.488 total
make -j1 -f posix.mak > /dev/null  5.02s user 0.14s system 93% cpu 5.529 total

Minimum: 5.423 (this PR) vs 5.446 (master)
Median: 5.497 (this PR) vs 5.503 (master)

This PR is at worst as fast as master.

andralex · 2017-08-01T14:54:26Z

@WalterBright typeToExpression may be confusing, "how to you mean I convert a type to an expression"?

andralex

@ibuclaw @redstar @klickverbot @MartinNowak could you please take a look

UplinkCoder · 2017-08-01T14:56:18Z

@andralex aliasSeq is a type. yet it can hold expressions.
this works via TypeExp.

WalterBright · 2017-08-01T23:21:13Z

src/ddmd/typesem.d

+    }
+}
+
+extern (C++) Expression typeToExpression(Type t)


No documentation.

WalterBright · 2017-08-01T23:21:26Z

src/ddmd/typesem.d

+    return v.result;
+}
+
+extern (C++) Expression typeToExpressionHelper(TypeQualified t, Expression e, size_t i = 0)


No documentation.

WalterBright · 2017-08-01T23:22:15Z

src/ddmd/initsem.d

+    }
+}
+
+public extern (C++) Expression initializerToExpression(Initializer i, Type t = null)


No documentation.

RazvanN7 · 2017-08-02T06:36:55Z

@WalterBright I added doc comments for those functions.

UplinkCoder · 2017-08-05T18:11:38Z

@RazvanN7 This broke newCTFE.
now I have to use a less descriptive name then toExpression.

UplinkCoder · 2017-08-05T22:06:34Z

@RazvanN7 How do I get the Initalizer.toExpression ?
Please tell me how to migrate to the visitor base approuch.

Thanks!

PetarKirov · 2017-08-05T23:00:12Z

initializerToExpression ?
(See the diff for this PR.)

RazvanN7 · 2017-08-07T09:46:31Z

@UplinkCoder As ZombineDev pointed out you have to use initializerToExpression for initializer nodes and typeToExpression for type nodes. I think these names are more expressive than toExpression.

RazvanN7 force-pushed the toExpression branch from 89c95d9 to 1290940 Compare July 31, 2017 12:19

RazvanN7 changed the title ~~Implement visitor for toExpression functions~~ [WIP] Implement visitor for toExpression functions Jul 31, 2017

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Jul 31, 2017

RazvanN7 force-pushed the toExpression branch from 1290940 to 5150541 Compare July 31, 2017 12:47

RazvanN7 changed the title ~~[WIP] Implement visitor for toExpression functions~~ Implement visitor for toExpression functions Jul 31, 2017

RazvanN7 force-pushed the toExpression branch 6 times, most recently from fa8daf3 to 8a7d383 Compare August 1, 2017 10:45

Implement visitor for toExpression functions

bc71b41

RazvanN7 force-pushed the toExpression branch from 8a7d383 to bc71b41 Compare August 1, 2017 11:00

RazvanN7 force-pushed the toExpression branch 2 times, most recently from 2a5fffe to b831f86 Compare August 1, 2017 11:37

andralex requested review from WalterBright and yebblies August 1, 2017 13:20

andralex assigned WalterBright Aug 1, 2017

andralex approved these changes Aug 1, 2017

View reviewed changes

WalterBright reviewed Aug 1, 2017

View reviewed changes

src/ddmd/typesem.d

}

}

extern (C++) Expression typeToExpression(Type t)

Copy link

Member

WalterBright Aug 1, 2017

Choose a reason for hiding this comment

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

No documentation.

WalterBright reviewed Aug 1, 2017

View reviewed changes

src/ddmd/initsem.d

}

}

public extern (C++) Expression initializerToExpression(Initializer i, Type t = null)

Copy link

Member

WalterBright Aug 1, 2017

Choose a reason for hiding this comment

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

No documentation.

Add documentation for *toExpression functions

1f12755

RazvanN7 force-pushed the toExpression branch from b831f86 to 1f12755 Compare August 2, 2017 06:35

WalterBright merged commit 411b55a into dlang:master Aug 4, 2017

Uh oh!

Conversation

RazvanN7 commented Jul 31, 2017 • edited by wilzbach Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jul 31, 2017

Bugzilla references

Uh oh!

UplinkCoder commented Jul 31, 2017

Uh oh!

WalterBright commented Jul 31, 2017

Uh oh!

RazvanN7 commented Aug 1, 2017

Uh oh!

RazvanN7 commented Aug 1, 2017

Uh oh!

RazvanN7 commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Aug 1, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

UplinkCoder commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

WalterBright Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

WalterBright Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UplinkCoder commented Aug 5, 2017

Uh oh!

UplinkCoder commented Aug 5, 2017

Uh oh!

PetarKirov commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RazvanN7 commented Jul 31, 2017 •

edited by wilzbach

Loading

RazvanN7 commented Aug 1, 2017 •

edited

Loading

andralex commented Aug 1, 2017 •

edited

Loading

UplinkCoder commented Aug 1, 2017 •

edited

Loading

RazvanN7 commented Aug 2, 2017 •

edited

Loading

PetarKirov commented Aug 5, 2017 •

edited

Loading