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

Cannot use class type in Map value type #14423

Open
BryantLam opened this issue Nov 12, 2019 · 5 comments
Open

Cannot use class type in Map value type #14423

BryantLam opened this issue Nov 12, 2019 · 5 comments

Comments

@BryantLam
Copy link

Related #14413

Bug. The map data structure does not accept class types for its value type.

I haven't tested using a class type for the key type yet. The same could be true.

use Map;

class MyInt {
  var x: int;
}

var m = new map(string, owned MyInt?);

proc always_set(key, value) {
  m[key] = value;
  writeln(m);
}

always_set("a", new MyInt?(1));
always_set("a", new MyInt?(2));
always_set("b", new MyInt?(11));
always_set("b", new MyInt?(22));
always_set("a", new MyInt?(3));
# chpl version 1.21.0 prerelease
$CHPL_HOME/modules/standard/Map.chpl:208: In function 'this':
$CHPL_HOME/modules/standard/Map.chpl:212: error: non-lvalue actual is passed to a 'ref' formal of init=()
@daviditen
Copy link
Member

always_set's formal argument value has const ref intent, and so it cannot be modified. Adding the ref intent so it is no longer const gets this code passing. We're looking into possibly adding an intent that is const ref if the argument is not modified and ref if it is in #14822.

@BryantLam
Copy link
Author

Thanks. You're right; that's rather subtle.

Is there intuition as to whether my original code should compile if owned had default ref-maybe-const intent in this case?

Is there any relevance here for the expiring-values optimization? My guess is "no" since this is a language semantic and not an optimization.

@daviditen
Copy link
Member

Yes, I would expect the original code to compile and work if ref-maybe-const were the default.

@mppf
Copy link
Member

mppf commented Jan 30, 2020

I would write

proc always_set(key, in value) {
  m[key] = value;
  writeln(m);
}

When owned and related records come in to play, I think it's better style to use the in intent to describe the ownership transfer/sharing.

Note also that if the function were declared as proc always_set(key, ref value), changes I am working now would cause it to be a compilation error to transfer the value out of value if it is non-nilable (but will allow it if it is passed with in intent).

Is there any relevance here for the expiring-values optimization?

With the in intent version, we could consider changing the = into a deinit/move. I do not think we have yet decided exactly what the copy elision for expiring values will do. But the in intent already solves the problem in this issue.

My guess is "no" since this is a language semantic and not an optimization.

The copy elision for expiring values is not an optimization. It is something that will be described in the language specification. However, we could decide to allow copy elision with = operator calls. I think my main hesitation there is that the LHS and RHS might be on different locales and in that event the = operator might do something more useful than a deinit/move would.

I don't have an issue yet proposing the details of the copy elision - #9490 mostly brings up tricky points - but we could discuss further there if you are interested.

@mppf
Copy link
Member

mppf commented Feb 6, 2020

I don't have an issue yet proposing the details of the copy elision - #9490 mostly brings up tricky points - but we could discuss further there if you are interested.

FWIW I've created #14860 to discuss the specific design for copy elision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants