Comments/C++headers/Changelog entry for lambda comparison by RazvanN7 · Pull Request #7809 · dlang/dmd

RazvanN7 · 2018-01-30T09:41:24Z

Part 2 of #7484

dlang-bot · 2018-01-30T09:41:25Z

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.

stefan-koch-sociomantic · 2018-01-30T10:55:16Z

src/dmd/lambdacomp.d

                    {
                        v.getConstInitializer.accept(this);
                    }
+                    // Otherwise, the function is uncomparable


This flow is a little confusing.
please let the buf.reset be a fall-through case.

it is a fall-through case, I just need it to be reset inside the first if-block if the var declaration isn't a manifest constant. I don't know now to do it differently without adding a label, which in my opinion would be worse.

Easy, use return. Then you only need to call buf.reset at the end.

it could also be {auto v = s.isVarDeclaration if (v && (v.storage_class & STC.manifest))}

In general if (a) {if (b) {c}} maps exactly to if (a && b) {c}

stefan-koch-sociomantic · 2018-01-30T10:55:38Z

src/dmd/lambdacomp.d

                    else
                        buf.reset();
                }
+                // Or it may be an enum


redundant comment

stefan-koch-sociomantic · 2018-01-30T10:56:23Z

changelog/lambdacomp.dd

+
+-> the lambda function arguments must not have a template instantiation as an explicit argument type. Any other argument types (basic, user-defined, template) are supported.
+
+-> the lambda function body must contain a single expression (no return statement) which contains only numeric values, manifest constants, enum values and arguments. If the expression contains local variables, function calls or return statements, the function is considered uncomparable.


more rational wanted.
why are the limitations the way they are ?

That's the point I got to. Later on, the limitations will be lifted (where possible).

stefan-koch-sociomantic

it's better but could be improved.

meta concern,
why would you serialize to a string and then compare that ?
it's expensive.

RazvanN7 · 2018-01-30T11:04:56Z

why would you serialize to a string and then compare that ?

To do parameter renaming and hopefully, later on, do some other computation (e.g. to be able to distinguish between a+b and b+a ). What is the alternative you are thinking?

jacob-carlborg · 2018-01-30T12:11:35Z

src/dmd/lambdacomp.d

+ * found when visiting the AST nodes to higher levels of the
+ * AST.
+ */
 enum ExpType


Seems like this can be private.

jacob-carlborg · 2018-01-30T12:15:41Z

src/dmd/func.d

+     * The serialization of this.
+     * != null only if tok == TOKreserved.
+     */
+    const(char)* serialization;


Could have a better description of what it's used for.

jacob-carlborg · 2018-01-30T12:17:16Z

changelog/lambdacomp.dd

+If the expression contains local variables, function calls or return statements,
+the function is considered uncomparable.
+
+These limitation will be lifted in the next release version.


These limitation will be lifted in the next release version.

I would recommend avoiding being this specific. There are not guarantees that those changes will make it into the next release. I would say something like: "These limitations might be lifted in the future".

jacob-carlborg · 2018-01-30T12:19:50Z

What happens if two lambdas are not comparable? Will compiler give an error or will you get back false in the comparison?
Does SerializeVisitor need to be exposed in a C++ header?

wilzbach · 2018-01-30T15:31:35Z

changelog/lambdacomp.dd

+correctly compare two lambdas, the following conditions must be
+satisfied:
+
+-> the lambda function arguments must not have a template instantiation


Until we have MarkDown support, the following is recommended:

$(UL $(LI ..) )

wilzbach · 2018-01-30T15:32:26Z

src/dmd/func.d


+    /**
+     * The serialization of this.
+     * != null only if tok == TOKreserved.


TOK.reserved

wilzbach · 2018-01-30T15:32:48Z

src/dmd/lambdacomp.d

+/***
+ * Compiler implementation of the
+ * $(LINK2 http://www.dlang.org, D programming language).
+ *


Describe the module here.

wilzbach · 2018-01-30T15:33:59Z

src/dmd/lambdacomp.d

@@ -1,3 +1,15 @@
+/***
+ * Compiler implementation of the
+ * $(LINK2 http://www.dlang.org, D programming language).


I know that this is the default header, but I don't like it very much. It's not helping anyone. The module description should be used to ... guess what ... describe the module.

wilzbach · 2018-01-30T15:36:18Z

src/dmd/lambdacomp.d

 }

+/**
+ * The serialize visitor visits a FuncLiteralDeclaration


$(REF dmd, func, FuncLiteralDeclaration) - in general there's also $(REF_ALTEXT myText, FuncLiteralDeclaration, dmd, func) if you prefer a custom text.

wilzbach · 2018-01-30T15:37:38Z

src/dmd/lambdacomp.d

    }

+    /**
+     * Entrypoint of the SerializeVisitor.


wilzbach · 2018-01-30T15:37:51Z

src/dmd/lambdacomp.d

    override void visit(FuncLiteralDeclaration fld)
    {
        if (fld.type.ty == Terror)
            return;


wilzbach · 2018-01-30T15:38:33Z

src/dmd/lambdacomp.d

                value.writestring("arg");
                value.print(i);
-                arg_hash.insert(key, strlen(key), value.extractString);
+                arg_hash.insert(&key[0], key.length, value.extractString);


wilzbach · 2018-01-30T15:39:24Z

src/dmd/lambdacomp.d

                    }
+                    // Otherwise, the function is uncomparable
                    else
                        buf.reset();


FWIW DStyle would require the else to use brackets here too.

wilzbach · 2018-01-30T15:42:12Z

src/dmd/lambdacomp.d

                    {
                        v.getConstInitializer.accept(this);
                    }
+                    // Otherwise, the function is uncomparable


Easy, use return. Then you only need to call buf.reset at the end.

WalterBright · 2018-01-31T01:47:17Z

src/dmd/lambdacomp.d

 import dmd.visitor;

+/**
+ * This enum is used to report the type of the expression


This enum is used to report can be excised with no change in meaning to the sentence.

WalterBright · 2018-01-31T01:47:49Z

src/dmd/lambdacomp.d

+/**
+ * This enum is used to report the type of the expression
+ * found when visiting the AST nodes to higher levels of the
+ * AST.


I don't know what visiting the AST nodes to the higher levels of the AST means.

WalterBright · 2018-01-31T01:50:25Z

src/dmd/lambdacomp.d


+/**
+ * The serialize visitor visits a FuncLiteralDeclaration
+ * AST node, serializing the implementation of the lambda


What does serialization of the implementation mean? Is a string produced? A hash?

wilzbach · 2018-02-05T15:06:01Z

Does SerializeVisitor need to be exposed in a C++ header?

I don't see any reason why it should. It's more or less a "private function of the DMD frontend implemented as a visitor" for the other backends. They only care about the IR.

What happens if two lambdas are not comparable? Will compiler give an error or will you get back false in the comparison?

false - it's the __traits(isSame)
In fact at the moment the comparability is severely limited and returning false if unsure is a very reasonable decision.

I'm going to merge this now, s.t. it appears on the 2.079 changelog.
Great work again @RazvanN7!

RazvanN7 requested review from dnadlinger and ibuclaw as code owners January 30, 2018 09:41

stefan-koch-sociomantic reviewed Jan 30, 2018

View reviewed changes

src/dmd/lambdacomp.d Outdated

else

buf.reset();

}

// Or it may be an enum

Copy link

stefan-koch-sociomantic Jan 30, 2018

Choose a reason for hiding this comment

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

redundant comment

stefan-koch-sociomantic reviewed Jan 30, 2018

View reviewed changes

stefan-koch-sociomantic suggested changes Jan 30, 2018

View reviewed changes

jacob-carlborg reviewed Jan 30, 2018

View reviewed changes

jacob-carlborg suggested changes Jan 30, 2018

View reviewed changes

wilzbach reviewed Jan 30, 2018

View reviewed changes

WalterBright reviewed Jan 31, 2018

View reviewed changes

RazvanN7 force-pushed the Lambda_part2 branch from 9e5c10c to 9811ad2 Compare January 31, 2018 10:04

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

0c2ad9c

RazvanN7 force-pushed the Lambda_part2 branch from 9811ad2 to 0c2ad9c Compare January 31, 2018 10:39

wilzbach added this to the 2.079.0 milestone Feb 5, 2018

wilzbach merged commit 3ad0b44 into dlang:master Feb 5, 2018


		-> the lambda function arguments must not have a template instantiation as an explicit argument type. Any other argument types (basic, user-defined, template) are supported.

		-> the lambda function body must contain a single expression (no return statement) which contains only numeric values, manifest constants, enum values and arguments. If the expression contains local variables, function calls or return statements, the function is considered uncomparable.

Uh oh!

Comments

Conversation

RazvanN7 commented Jan 30, 2018

Uh oh!

dlang-bot commented Jan 30, 2018

Bugzilla references

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

stefan-koch-sociomantic 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!

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!

stefan-koch-sociomantic left a comment

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 30, 2018

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!

jacob-carlborg commented Jan 30, 2018

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!

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!

wilzbach commented Feb 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RazvanN7 Jan 30, 2018 •

edited

Loading

stefan-koch-sociomantic Jan 30, 2018 •

edited

Loading