Condense build by marler8997 · Pull Request #10536 · dlang/dmd

marler8997 · 2019-11-05T04:17:42Z

condense build.d a bit more using the new initialization technique

dlang-bot · 2019-11-05T04:17:44Z

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

src/build.d

MoonlightSentinel · 2019-11-05T10:33:45Z

src/build.d

 Takes a lambda and returns a memoized function to build a dependecy object.
 */
-alias makeDep(alias Func) = memoize!(function() {
+alias makeDep(alias Func, T...) = memoize!(function(T args) {


I've tried some solutions to avoid explicit types separated from the identifiers but ran into different issues:

Explicit function: undefined identifier R|T

Suggested change

alias makeDep(alias Func, T...) = memoize!(function(T args) {

alias makeDep(R function(DepBuilder*, DependencyRef, T) Func, R, T...) = memoize!(function(T args) {

Rejected because it contains forward-references within the template parameter list
(This seems like an artificially restriction as e.g. template methods allow forward references to the return type but maybe I am underestimating the entailing implementation difficulties)

Extracting parameter types using traits:

Suggested change

alias makeDep(alias Func, T...) = memoize!(function(T args) {

alias makeDep(alias Func) = memoize!(function(Parameters!Func[2..$] args) {

Fails as it can't resolve the parameter types (probably caused by the cyclic dependency between Func and this lambda. Could a resolution fur such a scenario be realized in DMD with reasonable effort?)

?

I came up with something that seems to work...

Nice. It would be beneficial to conflate the builder and the dependency as this requires all parameter types to be spelled out.

Or at least have alias DependencyBuilder = MethodInitializer!DependencyRef*

meh, I'm fine with MethodInitializer!DependencyRef*. It's a bit verbose but that doesn't bother me as it will only be used once.

... as it will only be used once
Currently, but we can keep the full type anyway

Using DependencyBuilder adds a level of indirection. Now you have to look up what a DependencyBuilder is and then see that it's a MethodInitializer and then look up what that is. This can be worth it to save extra characters, but since it's only being used in one place, I don't see any benefit to it.

MoonlightSentinel · 2019-11-05T17:22:43Z

src/build.d

 }

+/** Initializes an object using a chain of method calls */
+struct MethodInitializer(T)


Why is this a template now?

Makes it more general, could be added to phobos. I try to structure the code in a way that general code could be factored out into a library.

PetarKirov

Nice work! I have some suggestions/questions:

PetarKirov · 2019-11-06T09:52:10Z

src/build.d

+
+alias makeDepImpl(alias Func, T...) = memoize!(function(T args) {
+    auto builder = MethodInitializer!DependencyRef(new DependencyRef());
+    Func(&builder, builder.obj, args);


Suggested change

Func(&builder, builder.obj, args);

Func(builder, builder.obj, args);

Good suggestion. Will include in the next PR.

PetarKirov · 2019-11-06T09:52:36Z

src/build.d

 */
-alias dmdExe = memoize!(function(string targetSuffix, string[] extraFlags...)
-{
+alias dmdExe = makeDepWithArgs!((MethodInitializer!DependencyRef* builder, DependencyRef dep, string targetSuffix, string[] extraFlags) {


Suggested change

alias dmdExe = makeDepWithArgs!((MethodInitializer!DependencyRef* builder, DependencyRef dep, string targetSuffix, string[] extraFlags) {

alias dmdExe = makeDepWithArgs!((MethodInitializer!DependencyRef builder, DependencyRef dep, string targetSuffix, string[] extraFlags) {

PetarKirov · 2019-11-06T09:53:55Z

src/build.d

-            "-of" ~ target,
+            "-of" ~ dep.target,
            "-vtls",
-            "-J"~env["G"],


As far as I can see, in order to use the output of versionFile target, we need to add the "-J"~env["G"] string import. Am I missing something?

Its part of flags["DFLAGS"], see

https://github.com/marler8997/dmd/blob/6082657531b5fa5da5e91c78d8b4a88afcfe403d/src/build.d#L798

Thanks, indeed I missed that part.

marler8997 force-pushed the condenseBuild branch 3 times, most recently from 1b509c3 to 4f27caa Compare November 5, 2019 06:52

MoonlightSentinel reviewed Nov 5, 2019

View reviewed changes

marler8997 force-pushed the condenseBuild branch 3 times, most recently from ec64fc1 to 6082657 Compare November 5, 2019 16:52

Condense build

6082657

MoonlightSentinel reviewed Nov 5, 2019

View reviewed changes

RazvanN7 approved these changes Nov 6, 2019

View reviewed changes

RazvanN7 merged commit 99e6ac8 into dlang:master Nov 6, 2019

PetarKirov approved these changes Nov 6, 2019

View reviewed changes

marler8997 deleted the condenseBuild branch November 6, 2019 16:48

	alias makeDep(alias Func, T...) = memoize!(function(T args) {
	alias makeDep(R function(DepBuilder*, DependencyRef, T) Func, R, T...) = memoize!(function(T args) {

	alias makeDep(alias Func, T...) = memoize!(function(T args) {
	alias makeDep(alias Func) = memoize!(function(Parameters!Func[2..$] args) {

	Func(&builder, builder.obj, args);
	Func(builder, builder.obj, args);

	alias dmdExe = makeDepWithArgs!((MethodInitializer!DependencyRef* builder, DependencyRef dep, string targetSuffix, string[] extraFlags) {
	alias dmdExe = makeDepWithArgs!((MethodInitializer!DependencyRef builder, DependencyRef dep, string targetSuffix, string[] extraFlags) {

Uh oh!

Comments

Conversation

marler8997 commented Nov 5, 2019

Uh oh!

dlang-bot commented Nov 5, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

MoonlightSentinel Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MoonlightSentinel Nov 5, 2019 •

edited

Loading

MoonlightSentinel Nov 5, 2019 •

edited

Loading

marler8997 Nov 5, 2019 •

edited

Loading