Skip to content

Add lvalue/rvalue traits #1261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 2, 2013
Merged

Conversation

denis-sh
Copy link
Contributor

  • Add lvalue/rvalue traits
    • rvalueOf/lvalueOf

(commits extracted from pull #954)



/**
Creates and lvalue or rvalue of type $(D T) for $(D typeof(...)) and
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "creates and".

@denis-sh
Copy link
Contributor Author

Is there a reason not to use documented unit test blocks here, or were they just not around when you first wrote that code?

The latter.


Fixed noted issues.

@9rnsr
Copy link
Contributor

9rnsr commented Apr 23, 2013

I think isLvalue template is not good. Contrary to the name, it does not work in all cases.

void main() {
    int x = 1;
    enum b = isLvalue!(1 + x);  // add expression makes rvalue but...
    // Error: variable x cannot be read at compile time  --> doesn't work!
}

We cannot fix the problem so it is an intrinsic limitation which comes from that isLvalue is implemented with template. I oppose to be adding such limited utility in standard library.

@9rnsr
Copy link
Contributor

9rnsr commented Apr 23, 2013

I can agree with adding lvalueOf and rvalueOf, but their implementations are not good currently.
@denis-sh , you can see the implementation of package template defaultInit in std.traits.

@denis-sh
Copy link
Contributor Author

@9rnsr, I just changed lvalue/rvalue traits according to compiler change.
I will look at defaultInit now.

@denis-sh
Copy link
Contributor Author

Rebased to fix merge conflicts.

Also I prefer my inout-hacking over defaultInit's one.

@denis-sh
Copy link
Contributor Author

To @9rnsr:

We cannot fix the problem so it is an intrinsic limitation which comes from that isLvalue is implemented with template. I oppose to be adding such limited utility in standard library.

So, we can't use isLvalue(i) as variable i cannot be referenced at compile time. We have to either change it (the error) or use the proposed interface isLvalue!i which can be later improved with compiler changes to support your proposed usage.

Or we can make expectLvalue public thus having this dangerous interface:

static assert( __traits(compiles, expectLvalue(i)));
static assert(!__traits(compiles, expectLvalue(i + 1)));
static assert(!__traits(compiles, expectLvalue(f)));

@9rnsr
Copy link
Contributor

9rnsr commented Apr 23, 2013

The limitation is comes from that a template cannot take a runtime expression.

template X(T...) {}
void main() {
    int n;
    alias X1 = X!(n);   // take a symbol `n`.
    alias X2 = X!(int);   // take a type `int`.
    alias X3 = X!(1);   // take a value `1`.
  //alias X4 = X!(n + 1);   // cannot take an expression `n + 1`
}

This is current language design of D, not a bug. So we cannot implement isLvalue in library.

Or we can make expectLvalue public thus having this dangerous interface:

It is useless, because expectLvalue itself would provide nothing.

@denis-sh
Copy link
Contributor Author

Other ugly but safe implementable interfaces:

Using mixin template:

mixin isLvalue!(`l_i`, q{i});
static assert(l_i);
mixin isLvalue!(`l_i1`, q{i + 1});
static assert(!l_i1);
static assert(!__traits(compiles, { mixin isLvalue!(`l_x`, q{x}); }));

Using string mixin:

static assert( mixin(isLvalue!q{i}));
static assert(!mixin(isLvalue!q{i + 1}));
static assert(!__traits(compiles, mixin(isLvalue!q{x})));

@denis-sh
Copy link
Contributor Author

It is useless, because expectLvalue itself would provide nothing.

Don't understand. I shown usage in examples.

@denis-sh
Copy link
Contributor Author

So we cannot implement isLvalue in library.

You mean "with this interface". What about other interfaces? Too ugly?

@9rnsr
Copy link
Contributor

9rnsr commented Apr 23, 2013

You mean "with this interface". What about other interfaces? Too ugly?

Yes, as you explained, we can provide it with the premise of using mixin. But it is too ugly. I cannot agree with it.

@denis-sh
Copy link
Contributor Author

But it is too ugly. I cannot agree with it.

Me too. What about the first of proposed "other interfaces", isLvalue(i)? I filled an enhancement to make it work Issue 9980.

@monarchdodra
Copy link
Collaborator

What's the status here? I understand that there are problems with isLvalue? If that is the case, maybe it would be worth splitting the pull? rvalueOf/lvalueOf (I think) could be of great help when writting static ifs.

A lot of code uses T.init instead, which is only about 90% accurate in what it is actually testing...

@denis-sh
Copy link
Contributor Author

denis-sh commented Aug 2, 2013

What about to make isLvalue undocumented for now?

@monarchdodra
Copy link
Collaborator

What about to make isLvalue undocumented for now?

Package maybe, but not undocumented, that never ends well.

Are there many use cases for isLvalue ? I mean, does phobos even really need it? It definitly needs lvalueOf, that's for sure.

@denis-sh
Copy link
Contributor Author

@9rnsr, I undocumented std.traits.isLvalue. Is anything else debatable in the pull? If not, lets merge.

@monarchdodra
Copy link
Collaborator

I undocumented std.traits.isLvalue.

Can't you make it package?

Is anything else debatable in the pull? If not, lets merge.

Yes: Given the discussions of @property and optional parenthesis, I don't think [EDIT:] isLvalue lvalueOf should be a @property (and, IMO, it is cleaner as a function). Making @property would prevent:

is(typeof(fun(&lvalueOf!T())));

Not supporting this syntax would just add one more pitfal in the pitfals that are initialization.

Ditto for rvalueOf: As a property, &rvalueOf!int would actually compile, whereas &rvalueOf!int() would result in a clean compile error (can't take address of rvalue)

@9rnsr
Copy link
Contributor

9rnsr commented Sep 23, 2013

I still think this isLvalue template is mis-designed and it should not be merged in Phobos.

@denis-sh
Copy link
Contributor Author

I still think this isLvalue template is mis-designed and it should not be merged in Phobos.

Of course is is. So it is undocumented now and used only in unittests, where it is needed. Or must I mark it private? I don't like that as somebody may be OK to use undocumented stuff for now as we are not ready to provide better solution soon.

@9rnsr
Copy link
Contributor

9rnsr commented Sep 23, 2013

You can test lvalueOf and rvalueOf without using isLvalue, as follows.

unittest
{
    void accepstsLvalue(T)(ref T lval);
    ...
        static assert(!__traits(compiles, acceptsLvalue(rvalueOf!T)));
        static assert( __traits(compiles, acceptsLvalue(lvalueOf!T)));
}

I recommend to remove isLvalue definition completely.

@denis-sh
Copy link
Contributor Author

To @monarchdodra:
I don't see the problem with these functions being @property as user isn't supposed to take the address of its return value. So for me you are complaining about a non-sense operation, sorry.
is(typeof(fun(&lvalueOf!T()))) is just and example of that chaos for me. As here one wants to pass rvalue of T* to fun and have to write __traits(compiles, fun(rvalueOf!(T*))) .

@denis-sh
Copy link
Contributor Author

I recommend to remove isLvalue definition completely.

OK. Will do now.

@denis-sh
Copy link
Contributor Author

@9rnsr, what about my f(inout T = default) workaround? Is it planned that property = value is rewritten to property(default) = value when value isn't convertible to T or just a compiler bug? There is no clear docs on the site and it's hard to find you related property fixing pulls and discussions.

@denis-sh
Copy link
Contributor Author

As my previous inout trick worked because of a compiler bug it will be funny if the situation repeats. )

@9rnsr
Copy link
Contributor

9rnsr commented Sep 23, 2013

@denis-sh Hmm, how about this to disallow setter syntax?

template rvalueOf(T)
{
    @property T rvalueOf(inout int = 1);
    @property T rvalueOf(inout int) @disable;       // Disallow rvalueof!T = 1; syntax
}

/// ditto
template lvalueOf(T)
{
    @property ref T lvalueOf(inout int = 1);
    @property ref T lvalueOf(inout int) @disable;   // Disallow lvalueof!T = 1; syntax
}

The error message for the setter syntax misuse is not so friendly, but it works.

@denis-sh
Copy link
Contributor Author

Hmm, how about this to disallow setter syntax?

As @monarchdodra noted, __traits(compiles, lvalueOf!Lhs = rvalueOf!Rhs) is what we need. My current solution works this way. As there is no cleaner variant and no relying on compiler bugs, let it stay so. Will add few more unittests now.

@denis-sh
Copy link
Contributor Author

Added __traits(compiles, lvalueOf!byte = 127)-like tests.

@@ -4085,6 +4090,55 @@ unittest
}


struct __InoutWorkaroundStruct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

private.

Also, could you document what you are working around? I have no idea what the whole inout __InoutWorkaroundStruct = __InoutWorkaroundStruct.init is about.

@denis-sh
Copy link
Contributor Author

To @monarchdodra:

private

I was unsure private default argument will work. I just checked, currently it works. @9rnsr, is it a bug or planned behavior?

Also, could you document what you are working around?

OK.

@monarchdodra
Copy link
Collaborator

I was unsure private default argument will work.

I hadn't thought of that actually. I don't know then. It seems to work as private.

enum hasElaborateAssign = is(typeof(S.init.opAssign(S.init))) ||
is(typeof(S.init.opAssign(lvalueOf))) ||
enum hasElaborateAssign = is(typeof(S.init.opAssign(rvalueOf!S))) ||
is(typeof(S.init.opAssign(lvalueOf!S))) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might as well also make these:

+        enum hasElaborateAssign = is(typeof(lvalueOf!S.opAssign(rvalueOf!S))) ||
+                                  is(typeof(lvalueOf!S.opAssign(lvalueOf!S))) || 

No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might as well also make these
...
No?

Yes we can but I'd like to let it be this way as T.init.func looks already common. If it is bad, lets discuss and replace it everywhere.

@monarchdodra
Copy link
Collaborator

This pull looks merge-ready to me. Any body have anything else to add? @9rnsr ?

@denis-sh
Copy link
Contributor Author

denis-sh commented Oct 2, 2013

@9rnsr, ping. Are you OK with this?

@9rnsr
Copy link
Contributor

9rnsr commented Oct 2, 2013

The __InoutWorkaroundStruct hack is really ugly, but I agree there's no other better way.

LGTM.

9rnsr added a commit that referenced this pull request Oct 2, 2013
@9rnsr 9rnsr merged commit 8b74811 into dlang:master Oct 2, 2013
denis-sh added a commit to denis-sh/phobos that referenced this pull request Oct 9, 2013
CRLF EOLs were introduced in ea72723 commit of pull dlang#1261.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants