Skip to content
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

Fix std.traits.hasElaborateAssign #954

Merged
merged 1 commit into from
Apr 17, 2013

Conversation

denis-sh
Copy link
Contributor

Fix hasElaborateAssign for static arrays.

Other changes are extracted to pulls #1260 and #1261.

http://d.puremagic.com/issues/show_bug.cgi?id=9956

@denis-sh
Copy link
Contributor Author

@9rnsr, is there any way to determine if there is a user defined opAssign, not compiler generated? It may be useful to weaken hasElaborateAssign for assigning .init.

@monarchdodra
Copy link
Collaborator

  • rvalueOf/lvalueOf Were HUGELY needed. I think they are a very welcome move.

@andralex
Copy link
Member

Related compiler bug: http://d.puremagic.com/issues/show_bug.cgi?id=9154

@monarchdodra
Copy link
Collaborator

Shouldn't rvalueOf/lvalueOf be rvalueof/lvalueof ?

It fits with the scheme of:

  • stringof
  • typeof
  • ...

@jmdavis
Copy link
Member

jmdavis commented Dec 22, 2012

Shouldn't rvalueOf/lvalueOf be rvalueof/lvalueof ?

I'd say no simply because they're not built into the language like stringof and typeof are, and they wouldn't follow Phobos' naming conventions if you did that.

int i = rvalueOf!int; // error, no actual value is returned
---
*/
@property T rvalueOf(T)() inout;
Copy link
Contributor

Choose a reason for hiding this comment

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

inout-qualified module level function should not be accepted. I think it is a compiler bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So how should I workaround this:

T f(T)() { assert(0); } // line 1

void g() {
    f!(inout int)();
}

Output:

main.d(1): Error: inout on return means inout must be on a parameter as well for T()
main.d(4): Error: template instance main.f!(inout(int)) error instantiating

?

Copy link
Contributor

Choose a reason for hiding this comment

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

In current, most better workaround I can suggest is:
https://github.com/D-Programming-Language/phobos/pull/842/files#L1R143

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good for functions, but isn't the fact that @property T defaultInit(inout int = 0); is a getter instead of being a setter is another compiler bug?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily. It takes an argument, so it can be used as a setter (and setters can return like you'd expect from opAssign), but the fact that it has a default argument means that you could use it as a getter. I could see an argument though that it doesn't really make sense for it be a getter, since it presumably does some setting as it does take an argument (default or not). So, I don't know what the correct answer is. It doesn't surprise me though if the compiler lets it be used as a getter.

Copy link
Contributor

Choose a reason for hiding this comment

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

In current, compiler does not treat @property function so much specially than other normal functions in its semantic analysis.
A @property attribute for a function declaration just means that provides a syntactic sugar for the usage of the function.
In other words, compiler does not recognize that a @property function is getter or setter.
It depends on how the function is used.

// Compiler recognizes just foo is annotated with `@property`, and has one default argument.
// Compiler *does not* recognize that foo is a getter or setter.
@property int foo(int n = 0) { return n; }

foo = 1;        // foo is used as a setter property. @property attribute for foo allows this *syntax*.
int n = foo;    // foo is used as a getter property. @property attribute for foo allows this *syntax*.
foo(1);         // foo is used as a normal function.
// compiler should be reject this syntax as a use case of foo. (But this is not implemented yet)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I found a good explanation about that.

A @property attribute just affects to the function name, not affects to the function declaration itself.
And, if the name annotated with @property is used like a variable, it is allowed. If the name is used like a function, it is disallowed.

Practically we might need some additional rules, but it is a principle explanation, in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. And yes, it will be really good to have this in docs, if you will be kind enough and will manage to find enough time.

@denis-sh
Copy link
Contributor Author

Rebased (just in the case someone cares).

@denis-sh
Copy link
Contributor Author

Am I understanding correct that there is no "implicitly convertible to lvalue" objects any more?

@jmdavis
Copy link
Member

jmdavis commented Jan 13, 2013

Am I understanding correct that there is no "implicitly convertible to lvalue" objects any more?

I'm not sure what you mean. I'm not aware of anything ever implicitly converting to an lvalue. However, struct literals are no longer lvalues. Perhaps thats' what you're referring to?

@denis-sh
Copy link
Contributor Author

Yes, you are not aware of the topic.
Previously (for struct S { }) S.init and S() weren't lvalues i.e. isLvalue from this pull returned false but were "implicitly convertible to lvalue" i.e. isImplicitlyConvertableToLvalue returned true:

template isImplicitlyConvertableToLvalue(alias a) if(!is(a))
{ enum isImplicitlyConvertableToLvalue = __traits(compiles, (ref x) { } (a)); }

@dnadlinger
Copy link
Member

Unfortunately doesn't merge any longer. Has the confusion been cleared up in the meantime, and could you please rebase the pull request? I'd hate to see all your work on static array handling go to waste.

@denis-sh
Copy link
Contributor Author

Has the confusion been cleared up in the meantime...

I don't think so.

and could you please rebase the pull request?

Done.

I'd hate to see all your work on static array handling go to waste.

Some people hate black people, some people hate Russians, Phobos maintainers hate static arrays.
Byt the way, my work is not in waste. Because of lack of lots of things and as the second thing Phobos maintainers hate is to try to understand your arguments and respect your position Phobos isn't suitable as a standard library for me already for a long time and I use it only with my phobos-additions project.

@ghost
Copy link

ghost commented Mar 26, 2013

Some people hate black people, some people hate Russians, Phobos maintainers hate static arrays.

This is way out of line.

@andralex
Copy link
Member

Yah, I meant to reply to that. This kind of drama and hyperbole may seem cool at the originating end but at the receiving end is just inappropriate, so at best we should refrain from it. Regarding the matter at hand, @denis-sh you'd help a lot if you made collaboration just a little easier. Thanks!

@denis-sh
Copy link
Contributor Author

Rebased extracting everything but fixes and small refactoring to other pulls (see description).

@monarchdodra
Copy link
Collaborator

The third commit (the "money" commit) looks good to me. It fixes yet another issue on a trait that forgets that static arrays can have complex behavior. We needs these fixes merged in sooner rather than later.

The other two commits feels only like code churn though: I'd feel more comfortable if you changed every instance of typeof(S.tupleof) in the file in one corrective swoop. As for "__traits(compiles" vs "is(typeof", there is (AFAIK), no consensus in one over the other. Changing the existing code is just edit warring, IMO.

@9rnsr
Copy link
Contributor

9rnsr commented Apr 17, 2013

The change in hasElaborateAssign for static array type looks good to me.

Other two changes (replacing to __traits(compiles) and FieldTypeTuple!S) are not related to the issue described in the summary, and as far as I see, they does not have any effect. I'd recommend to just remove them.
.

@denis-sh
Copy link
Contributor Author

I see, there is really no consensus, it was just IMO, sorry. Removed __traits(compiles) commit.

But replacing of typeof(S.tupleof) with FieldTypeTuple!S has some arguments, extracted to #1262.

So there is the only commit waiting here...

@9rnsr
Copy link
Contributor

9rnsr commented Apr 17, 2013

LGTM. Please wait auto tester green.

@dnadlinger
Copy link
Member

LGTM, waiting for the tester to go green.

9rnsr added a commit that referenced this pull request Apr 17, 2013
@9rnsr 9rnsr merged commit e45e6ce into dlang:master Apr 17, 2013
@ghost
Copy link

ghost commented Apr 18, 2013

Is there a bug report for this?

@denis-sh
Copy link
Contributor Author

Is there a bug report for this?

No.

@ghost
Copy link

ghost commented Apr 18, 2013

No.

Ok, but in the future do make a bug report if one does not exist, and in the commit message add fixes issue ####, replacing #### with the issue number. Thanks.

@dnadlinger
Copy link
Member

(for the fix to show up in the changelog)

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.

6 participants