-
-
Notifications
You must be signed in to change notification settings - Fork 701
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
Add std.typecons.HeadConst (Continuation of #2881) #3862
Conversation
if (!is(T == const) && !is(T == immutable)) | ||
{ | ||
private T HeadConst_value; | ||
mixin Proxy!HeadConst_value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I comment this line out and add ref
to the return type of HeadConst_get()
, all of your unittests still pass.
How about adding another test or two documenting why Proxy
is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added the relevant test. Further attempts to break it are much appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A naive viewpoint is that the alias this
that returns an rvalue should be enough to implement head constness. Could you please document (privately and if needed to the user as well) why the proxy is necessary?
Since you asked... void main(string[] args) {
struct Fail {
this(bool fail) { }
}
HeadConst!Fail fail = true; // Error: cannot access frame pointer of main.Fail
} |
Hm, I don't know how to make it work with non-static nested structs or classes. You should be able to do: |
Yes, that works.
Can you at least detect such cases and print a better error message? |
de5c444
to
0ec76f9
Compare
Done. |
if (__traits(compiles, T(args))) | ||
{ | ||
static assert((!is(T == struct) && !is(T == union)) || !isNested!T, | ||
"Nested type " ~ fullyQualifiedName!T ~ " must be constructed " ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Nested type T must be marked as static or constructed explicitly at the call-site", etc.
Since the error triggers even when the context is not actually being used for anything, just marking the type as static
is going to be the best fix most of the time (at least with the way I tend to use nested types).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "nested type" is usually reserved for non-static nested types. If the user didn't make it static then it should be assumed that this is intentional. Error messages are not tutorials.
If so, then this is specific to D - C#, for instance has Nested Types which cannot capture their context at all.
If |
We should not consider C# semantics in our error messages.
Performance and optimization is a complete red herring. Telling the user to mark the type as static is not an option because such an action breaks code that uses the context. The current message is correct in all cases. |
C# is just one example of many. I looked, and even the D language specification uses "nested" in a way that encompasses
The point I was making, is that a user has no real incentive to mark a nested type as
My proposed message simply presents the user with an additional option. This will cause no problems for the user, unless he doesn't know what But, if you still want to argue I will let someone else answer, as I do not consider this detail to be a blocker. At the moment, I see no other problems with this PR - except that, considering how hard it is to plug all the loopholes in a scheme like this, it might be best to run it through |
I've disambiguated the error message a little.
It is the conservative estimate. Users that don't know the difference can safely follow the advice. |
Looks good. Thoughts on |
Last time we discussed using std.experimental as a staging area for additions to existing modules it was met with a lot of resistance. edit: A relevant consideration is that the interface of |
Hmm. Seems like the biggest concern was uncertainty that anything would ever actually get migrated out of I think |
I will note that most of the resistance was for requiring all changes to go through std.experimental. I think it's fine to put new pieces of a module into std.experimental (make sure you public import std.typecons from std.experimental.typecons). |
Fix ticket 13796
|
assert(sboth.i == 42); | ||
|
||
sboth.i = 24; | ||
assert(sboth.i == 24); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a C++ const struct field, shouldn't i
stay constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think preventing assignment for structs when fields can still be assigned to makes much sense.
Looks sensible. How about we name it Final as in Java's final (i.e. write once)? |
@DmitryOlshansky See the comments in the previous PR. |
I originally called it |
@JakobOvrum I'd still suggest Final if only for the reason that while HeadConst is pedantically correct it's an awful lot to type for a common need and for Final to be useful if it must not be a chore to use. |
Another vote for |
But, calling it In the longer to term what I would like to see is:
and either:
Symmetry in API design is important. Please don't break it, just to appeal to Java users. |
Also anyone who doesn't like the correct name or doesn't feel like typing such a long name can just put alias in his code to |
@ZombineDev I don't see this problem - just say that Final is for write once variables. |
@DmitryOlshansky I don't think that an outsider would want to use Another reason why
B)
|
I have to agree with @ZombineDev |
Yes, write one variables are like that - you can't rebin the name but you can overwrite all fields if that is allowed by its structure (i.e. they are all publicly accessible) |
Personally I don't care for a name just that I for one would not type HeadConst anywhere suitable the semantical gains vs visual noise ratio is too low. So if everyone is for HeadConst so be it. |
Same here. |
Technically this should be called |
@ntrel
Obviously we need The only meaning that |
This pull allows |
+ | ||
+ Head-const variables cannot be directly mutated or rebound, but references | ||
+ reached through the variable are typed with their original mutability. | ||
+ It is equivalent to $(D final) variables in D1 and Java, as well as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think therefore Final
would be a better choice than HeadConst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think therefore
Final
would be a better choice thanHeadConst
Someone needs to make an executive decision about the name. It has already been debated (and changed once), with about a 50% / 50% split of votes:
For HeadConst
: ZombineDev, JackStouffer, schuetzm, Dicebot
For Final
: JakobOvrum, DmitryOlshansky, tsbockman (me), andralex
(Click the links if you want to skim the earlier discussion.)
OK. A good step forward is to put this in |
For more detail: we should define |
Already exists :) |
This has been preapproved for more than two months - is someone interested in adopting the PR? |
I went ahead -> #4755 ;-) |
Ressurection of #2881. Fixes the issue @monarchdodra pointed out by using std.typecons.Proxy.
Github didn't give me the option to reopen the old PR after I force pushed the new changes.