Implement initial version of lambda comparison by RazvanN7 · Pull Request #7484 · dlang/dmd

RazvanN7 · 2017-12-21T13:07:52Z

This PR implements the first draft of the lambda comparison project. For the moment, it is possible to compare lambda functions :

-> which use in their body only parameters, IntegerExps and enums (no runtime variables)
-> that have user defined types as parameter types

A lambda function which has runtime variables or function calls in its body will be considered uncomparable for the moment. I plan on working on that case.

The current PR is just a proof of concept and I am sure that there are a lot of corner cases that I've missed and if you identify such cases please report them. I plan an adding some tests in the next commit and also try to see if function calls can be supported by putting the mangled name of the function in the lambda serialization.

dlang-bot · 2017-12-21T13:07:53Z

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.

JinShil · 2017-12-21T13:13:17Z

What's the use case motivating this?

RazvanN7 · 2017-12-21T13:16:19Z

@JinShil In phobos there are a lot of algorithms which may apply some optimizations if the given range is sorted. In order to check whether the range is sorted with a certain predicate a lambda comparison operation is required which is currently achieved by comparing string lambda's; this technique is buggy since it only does string comparison. By adding support in the compiler for lambda comparison this will enable safe comparison of predicates

wilzbach · 2017-12-21T13:24:14Z

What's the use case motivating this?

A good example of my head is this recent PR which makes {min,max}Element faster dlang/phobos#4265
For an identity map function, the algorithm is 6x faster with LDC.
So how do we check for identity?
Currently the only ways are (a) string lambdas (enum isIdentity = map == "a";) or (b) using many overloads (that's what we did with {min,max}Element eventually).
I really look forward to use the new trait for such optimizations :)

RazvanN7 · 2017-12-21T13:28:54Z

Does anyone have any idea why this particular line [1] makes compiling druntime impossible?

[1] https://github.com/dlang/dmd/pull/7484/files#diff-b601037010863a0a6d136649d1e4a5d6R195

Is there another way to obtain the mangled name of a Dsymbol which does not crash druntime?

I figured it out: it seems that in order to do the mangling, semantic3 must have been performed and for some situations that is not the case.

wilzbach · 2017-12-21T13:31:55Z

What's the use case motivating this?

Oh there's also the movement to phase out string lambdas, see e.g. this old NG post and IIRC comparability was one of the last blocking points.

jacob-carlborg · 2017-12-21T20:39:33Z

I figured it out: it seems that in order to do the mangling, semantic3 must have been performed and for some situations that is not the case.

The semantic analysis of the .mangleof property is done in the first semantic phase, as far as I understand.

RazvanN7 · 2017-12-22T11:18:28Z

@jacob-carlborg The mangleof property is present only for types and expressions. For functions there is the mangleString field which can be interogated, but for Dsymbols (or AggregateDeclarations to be more specific) I haven't anything but the mangleToBuffer function.

wilzbach · 2018-01-15T15:33:49Z

Nope. All CIs compile DMD from source before running the testsuite. So the error is on your side.

Closing and opening for SemaphoreCI to appear.

andralex · 2018-01-23T01:40:46Z

cc @WalterBright

andralex

nice work please fix style

andralex · 2018-01-29T15:25:42Z

src/dmd/traits.d

+        {
+            if (auto t = isDsymbol(oarg))
+            {
+                if(auto td = t.isTemplateDeclaration())


space after if

andralex · 2018-01-29T15:30:04Z

Will pull this, please add to the spec appropriately.

UplinkCoder · 2018-01-29T17:09:26Z

There are no comments!

wilzbach · 2018-01-29T17:14:28Z

And no changelog entry / spec PR :(

wilzbach · 2018-01-29T17:15:43Z

src/dmd/lambdacomp.d

@@ -0,0 +1,227 @@
+module dmd.lambdacomp;


No ddoc header - please have a look at the header files.

wilzbach · 2018-01-29T17:16:31Z

src/dmd/lambdacomp.d

+    StringTable arg_hash;
+    Scope* sc;
+    ExpType et;
+    Dsymbol d;


wilzbach · 2018-01-29T17:17:34Z

src/dmd/lambdacomp.d

+            auto fparam = Parameter.getNth(tf.parameters, i);
+            if (fparam.ident !is null)
+            {
+                auto key = fparam.ident.toString().ptr;


FWIW .ptr is deprecated in @safe code: https://dlang.org/changelog/pending.html#ptr-safe-end-of-deprecation

wilzbach · 2018-01-29T17:18:27Z

src/dmd/lambdacomp.d

+                OutBuffer value;
+                value.writestring("arg");
+                value.print(i);
+                arg_hash.insert(key, strlen(key), value.extractString);


You already call toString above and know the length of key

oh, ok thanks

wilzbach · 2018-01-29T17:19:30Z

src/dmd/traits.d

        auto o1 = (*e.args)[0];
        auto o2 = (*e.args)[1];
+
+        FuncLiteralDeclaration isLambda(RootObject oarg)


ibuclaw · 2018-01-29T18:29:06Z

And no C++ header updates.

andralex · 2018-01-29T18:52:20Z

@RazvanN7 and I discussed this during our regular meeting. @RazvanN7 in addition to the spec update, please also add a changelog entry. @ibuclaw what C++ header updates are needed?

ibuclaw · 2018-01-29T18:56:31Z

A new field was added to FuncLiteralDeclaration.

https://github.com/dlang/dmd/pull/7484/files#diff-ca782fb8b1ce12422a29fed42ffe0cadR2921

RazvanN7 · 2018-01-30T10:16:27Z

Doc update : dlang/dlang.org#2146

RazvanN7 requested review from CyberShadow, MartinNowak, WalterBright and wilzbach as code owners December 21, 2017 13:07

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

RazvanN7 force-pushed the Lambda_comparison branch from c481019 to edf5d0c Compare December 21, 2017 13:10

dlang-bot added Review:Needs Rebase Review:Needs Work and removed Review:Needs Rebase Review:Needs Work labels Jan 1, 2018

RazvanN7 force-pushed the Lambda_comparison branch 9 times, most recently from 551f5d2 to e6a74ee Compare January 5, 2018 10:10

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jan 5, 2018

RazvanN7 force-pushed the Lambda_comparison branch from e6a74ee to edb2bec Compare January 12, 2018 10:17

wilzbach closed this Jan 15, 2018

wilzbach reopened this Jan 15, 2018

RazvanN7 force-pushed the Lambda_comparison branch 2 times, most recently from 3e2e559 to 1eb0aa4 Compare January 22, 2018 09:56

RazvanN7 changed the title ~~[WIP] Add initial implementation of lambda comparison~~ Implement initial version of lambda comparison Jan 22, 2018

RazvanN7 force-pushed the Lambda_comparison branch from 57727d2 to 2a293ea Compare January 22, 2018 11:00

andralex mentioned this pull request Jan 23, 2018

Fix Issue 18280 - std.algorithm.comparison.cmp for non-strings should call opCmp only once per item pair dlang/phobos#6056

Merged

andralex approved these changes Jan 29, 2018

View reviewed changes

src/dmd/traits.d Outdated

{

if (auto t = isDsymbol(oarg))

{

if(auto td = t.isTemplateDeclaration())

Copy link

Member

andralex Jan 29, 2018

Choose a reason for hiding this comment

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

space after if

Implement lambda comparison

126a56e

RazvanN7 force-pushed the Lambda_comparison branch from 2a293ea to 126a56e Compare January 29, 2018 15:28

andralex added Merge:auto-merge and removed Review:WIP Work In Progress - not ready for review or pulling labels Jan 29, 2018

dlang-bot merged commit c048abb into dlang:master Jan 29, 2018

wilzbach added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Jan 29, 2018

wilzbach reviewed Jan 29, 2018

View reviewed changes

src/dmd/lambdacomp.d

@@ -0,0 +1,227 @@

module dmd.lambdacomp;

Copy link

Contributor

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

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

No ddoc header - please have a look at the header files.

wilzbach reviewed Jan 29, 2018

View reviewed changes

src/dmd/lambdacomp.d

StringTable arg_hash;

Scope* sc;

ExpType et;

Dsymbol d;

Copy link

Contributor

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

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

private?

wilzbach reviewed Jan 29, 2018

View reviewed changes

This was referenced Jan 30, 2018

Comments/C++headers/Changelog entry for lambda comparison #7809

Merged

Update docs to highlight lambda comparison dlang/dlang.org#2146

Merged

wilzbach mentioned this pull request Jan 30, 2018

Release v0.6.0 dlang-community/dfmt#320

Closed

Uh oh!

Comments

Conversation

RazvanN7 commented Dec 21, 2017

Uh oh!

dlang-bot commented Dec 21, 2017

Bugzilla references

Uh oh!

JinShil commented Dec 21, 2017

Uh oh!

RazvanN7 commented Dec 21, 2017

Uh oh!

wilzbach commented Dec 21, 2017

Uh oh!

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

Uh oh!

Uh oh!

wilzbach commented Dec 21, 2017

Uh oh!

jacob-carlborg commented Dec 21, 2017

Uh oh!

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

Uh oh!

Uh oh!

wilzbach commented Jan 15, 2018

Uh oh!

andralex commented Jan 23, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

andralex Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

andralex commented Jan 29, 2018

Uh oh!

UplinkCoder commented Jan 29, 2018

Uh oh!

wilzbach commented Jan 29, 2018

Uh oh!

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jan 30, 2018 • 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!

wilzbach Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jan 29, 2018

Uh oh!

andralex commented Jan 29, 2018

Uh oh!

ibuclaw commented Jan 29, 2018

Uh oh!

RazvanN7 commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

RazvanN7 commented Dec 21, 2017 •

edited

Loading

RazvanN7 commented Dec 22, 2017 •

edited

Loading

RazvanN7 Jan 30, 2018 •

edited

Loading