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
[New DIP] Copy constructor #129
Conversation
DIPs/DIP1xxx-rn.md
Outdated
this.b = another.b; | ||
} | ||
|
||
@implcit this(ref A another) // error typeof(this) != C, but might be accepted in the future |
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.
Typo: implicit
So, a reminder for anyone reading this thread who is considering a new DIP:
https://github.com/dlang/DIPs/blob/master/PROCEDURE.md#development-stage |
DIPs/DIP1xxx-rn.md
Outdated
A a; | ||
this(this) | ||
{ | ||
this.a.a = a.a + 2; // modifying immutable error or ok? |
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.
To me this looks weird when +=
exists.
DIPs/DIP1xxx-rn.md
Outdated
void fun() | ||
{ | ||
A b; | ||
a.m.aquire(); |
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.
s/aquire/acquire/
DIPs/DIP1xxx-rn.md
Outdated
The following link exhibits the proposed syntax and the necessary grammar changes in order to support | ||
the copy construct: | ||
|
||
https://github.com/dlang/dlang.org/pull/2414 |
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.
Why not something similar to C++? It's not even discussed here. Also, making the syntax a link to somewhere else makes it harder to read when it's an important part of the DIP.
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 link is added in order to show exactly what the changes to the grammar are and also because there is no nice way to explicitly express the grammar in the md format.
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.
It makes it a lot harder to read and understand what's being proposed.
I still don't understand why such a radical departure from C++ syntax, given that AFAIK is the only language with a copy constructor.
DIPs/DIP1xxx-rn.md
Outdated
|
||
#### Requirements | ||
|
||
1. The `structName` type needs to be identical to `typeof(this)`; an error is issued otherwise. This |
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.
This took me a while to understand. It'd probably be best to say that the type of the parameter to the copy constructor needs to be identical to typeof(this)
.
DIPs/DIP1xxx-rn.md
Outdated
|
||
#### Interaction with `alias this` | ||
|
||
There are situations in which a struct defines both an alias this and a copy constructor and assignemnts |
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.
typo on "assignment"
DIPs/DIP1xxx-rn.md
Outdated
### Overview of this(this) | ||
|
||
The postblit function was designed as a non-overloadable, non-qualifiable | ||
function. However, up until recently [1], the compiler did not reject the |
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.
This section needs reworking. It is irrelevant and confusing to talk about how things were in the past. State what the current problem is.
DIPs/DIP1xxx-rn.md
Outdated
} | ||
``` | ||
|
||
When `B c = b;` is encountered, the compiler does the following steps: |
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 compiler does the following steps
Careful in wording: it is irrelevant what a particular language implementation does. Write things down in terms of semantics. (in this particular case, the compiler isn't doing any of these steps. It is the generated executable that does them)
DIPs/DIP1xxx-rn.md
Outdated
After `step 1`, the object `c` has the exact contents as `b` but it is not | ||
initialized (the postblits still need to fix it) nor uninitialized (the field | ||
`B.a` does not have its initial value). From a type checking perspective | ||
this is a problem because the compiler will decide that the assignment |
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.
same here: simply remove "the compiler will decide that".
DIPs/DIP1xxx-rn.md
Outdated
inside A's postblit is breaking immutability. This makes it impossible to | ||
postblit objects that have `immutable`/`const` fields. To alleviate this problem | ||
we can consider that after the blitting phase the object is in a raw state, | ||
therefore uninitialized; this will allow the compiler to consider the first |
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.
"this way, the first assignment of B.a.a
will be considered an initialization."
DIPs/DIP1xxx-rn.md
Outdated
we can consider that after the blitting phase the object is in a raw state, | ||
therefore uninitialized; this will allow the compiler to consider the first | ||
assignment of `B.a.a` as an initialization. However, after this step the field | ||
`B.a.a` is considered initialized, therefore how is the compiler suppose to type |
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.
reword after removing "compiler"
DIPs/DIP1xxx-rn.md
Outdated
#### Interaction with `alias this` | ||
|
||
There are situations in which a struct defines both an alias this and a copy constructor and assignemnts | ||
to variables of the struct type may lead to ambiguities: |
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.
"both an alias this and a copy constructor, and for which assignments to variables "
(add comma, and fix spelling)
DIPs/DIP1xxx-rn.md
Outdated
|
||
## Breaking Changes and Deprecations | ||
|
||
The proposed changes will not break any code since the feature is entirely new to the language, |
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.
note my comment about @implicit
breakage.
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.
It'd be nice to have a discussion of the extra complexities this brings. For example, with this DIP, a = b;
may modify b
, severely impacting legibility and reasoning. Is it allowed to only declare a copy ctor, @implicit this(ref typeof(this));
, such that the implementation is opaque? (e.g. in a .di
file)
How can this new language feature be abused?
Etc.
DIPs/DIP1xxx-rn.md
Outdated
|
||
Currently, unions may not define postblits, destructors and invariants. Given this | ||
situation, it is unclear what the benefits would be if unions would be allowed to | ||
declare copy constructors. |
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.
Mention explicitly somewhere that copy constructors are not proposed for classes.
DIPs/DIP1xxx-rn.md
Outdated
#### Semantics | ||
|
||
The copy constructor typecheck is identical to the constructor one [6][7]. If a copy constructor | ||
has an empty body, the source is initialized to its `.init` property. |
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'm sure here you mean that the "destination" is initialized to .init
.
If things are the same as the normal constructor, why does this need to be mentioned at all? E.g. what happens if the copy ctor body is not empty?
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.
Can I also @disable
the copy constructor, such that a = b;
gives a compile error?
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.
What is the order of copy ctor calls when a struct contains a struct?
struct A { int a; }
struct B {
A aaa;
int b;
@implicit this (ref typeof(this) rhs) {
// has `aaa` already been copied with copy-ctor at this point?
}
}
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.
There is no order, because the user is in charge of copying all the fields of B
. If inside the copy constructor there is a line "this.aaa = rhs.aaa", then A's copy constructor will get called (if A defines a copy-ctor)
DIPs/DIP1xxx-rn.md
Outdated
```d | ||
struct A | ||
{ | ||
@implicit this(ref A another) {} // 1 - mutable source, mutable destination |
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 you should mention somewhere very explicitly that this allows modification of the source.
This means that from now on, a = b;
in general might modify b
. Do we really want that?
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.
If it was not wanted then wouldn't the author make another
a const ref
or?
@JohanEngelen thank you very much for your extensive review! |
d419135
to
6a1dea3
Compare
DIPs/DIP1xxx-rn.md
Outdated
are encountered exactly in this order. Although `@implicit` looks like a user defined attribute, it is not | ||
considered one to avoid breaking user code that already uses it. The use of `@implicit` in conjunction with `this` | ||
benefits from the advantage of declaring the copy constructor in an expressive manner without adding any new | ||
keywords to the language. |
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.
Doesn't "Although @implicit
looks like a user defined attribute, it is not considered one to avoid breaking user code that already uses it" imply (heh) that this effectively adds a keyword to the language?
DIPs/DIP1xxx-rn.md
Outdated
phases (even if the threads don't actually touch each other's data) at the cost | ||
of performance. Python implements a global interpreter lock and it was proven | ||
to cause unscalable high contention; there are ongoing discussions of removing it | ||
from the Python implementation [5]. |
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.
This will an Markdown document on the web. Can't we use an actual link?
[Python-implementation][python-implementation]
...
[python-implementation]: http:...
There's also no precedent of this style in the other DIPs, is there?
DIPs/DIP1xxx-rn.md
Outdated
The following link exhibits the proposed syntax and the necessary grammar changes in order to support | ||
the copy construct: | ||
|
||
https://github.com/dlang/dlang.org/pull/2414 |
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.
Can't we include a diff here, s.t. the DIP is self-contained? (in case the PR gets force-pushed or modified later)
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 agree, following the link and looking at the diff mostly displays ddoc noise. Showing the new grammar rules here would be a lot nicer.
DIPs/DIP1xxx-rn.md
Outdated
|
||
alias fun this; | ||
|
||
@implicit this(ref A another) immutable {} |
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.
should type be ref B
?
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.
no, it would be an error that way. The type of the copy constructor parameter has to be typeof(this)
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.
Hmm... I could swear struct B (in code below) had @implicit this(ref A another) immutable {} and I meant to comment on that :) Probably that code was squashed.
Never mind, now it's OK anyway.
5661f8a
to
79fff39
Compare
79fff39
to
a05eee1
Compare
I've renamed some variables to make the examples easier to read.
I've made a pull with some minor tweaks here: RazvanN7#1. |
Tweak examples, remove missing link
5559586
to
7815bc3
Compare
Making the |
DIPs/DIP1xxx-rn.md
Outdated
### Syntax | ||
|
||
The following link exhibits the proposed syntax and the necessary grammar changes in order to support | ||
the copy construct: |
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.
You mean " copy constructor", don't you?
DIPs/DIP1xxx-rn.md
Outdated
|
||
A declaration is a copy constructor declaration if it is a constructor declaration annotated with | ||
the `@implicit` attribute and takes only one parameter by reference that is of the same type as | ||
`typeof(this)`. `@implicit` is a compiler recognised attribute like `@safe` or `nogc` and it can be |
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.
@nogc
(with @
)
DIPs/DIP1xxx-rn.md
Outdated
|
||
### Semantics | ||
|
||
This sections discusses all the aspects regarding the semantic analysis and interaction with other |
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.
section (singular)
DIPs/DIP1xxx-rn.md
Outdated
### Semantics | ||
|
||
This sections discusses all the aspects regarding the semantic analysis and interaction with other | ||
components of the copy constructor |
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.
Dot missing.
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.
and "This sections"
DIPs/DIP1xxx-rn.md
Outdated
alias fun this; | ||
|
||
@implicit this(ref B another) immutable {} | ||
|
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.
probably superfluous empty line.
DIPs/DIP1xxx-rn.md
Outdated
is problematic in situations when the body is missing. In conclusion, `opAssign` can be generated from the | ||
copy constructor when the `struct` contains only assignable (mutable) fields. | ||
|
||
2. The copy constructor signature is : `@implicit this(ref $q1 S rhs) $q2`, where `q1` and `q2` represent the |
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.
Space in the front of colon.
DIPs/DIP1xxx-rn.md
Outdated
void main() | ||
{ | ||
A a; | ||
a = fun(); // copy constructor gets called |
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.
Wouldn't this be a breaking change to require the copy constructor in this case? Right now move semantics are used, so this can be done without a postblit constructor @disable this(this);
. Otherwise what rules are going to be used to determine whether a move can simply be done or a copy constructor needs to be called?
struct A
{
@disable this(this);
int a;
}
A func()
{
return A(0);
}
void main()
{
A a;
a = func(); // ok
}
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.
You are right, the value is moved, but in my example the function returns an lvalue and a call to the copy constructor is issued on callee site. This is explained in the next paragraph.
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.
Yes but why would it do a Copy if it can just do a Move instead? In this instance when the copy constructor is the same type.
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 copy constructor may be used for conversions:
struct A
{
int[] a;
@implicit this(ref immutable A rhs) {}
}
A func(immutable A a)
{
return a;
}
void main()
{
A a;
a = func();
}
DIPs/DIP1xxx-rn.md
Outdated
```d | ||
struct A | ||
{ | ||
@implicit this(ref inout A another) immutable {} |
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.
Why does modifier immutable
allow the assignment to mutable a
below?
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.
This is indeed a mistake. The types are reversed : a,b,c should be immutable and the r's mutable, const, immutable
DIPs/DIP1xxx-rn.md
Outdated
Note that structs that do not define a postblit explicitly but contain fields that | ||
define one have a generated postblit. From the copy constructor perspective it makes | ||
no difference whether the postblit is user defined or generated: | ||
|
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 this will not compose very well with structs that contain legacy fields that have a postblit. Can't the implicitly generated postblit just be omitted instead?
What about the reverse: a struct that has a postblit, but also fields with copy constructor?
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.
It's the same, postblit and copy constructor just can't go together. I've thought about preferring copy constructor over postblit, but the postblitt is essentially broken and must be deprecated. The code that defines a postblit can simply transform it into a copy constructor my changing the declaration.
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.
It is ok that postblit and copy constructor don't coexist in a struct, but fields could still just have a postblit that is used when the copy constructor of the outer struct initializes the field.
DIPs/DIP1xxx-rn.md
Outdated
struct A | ||
{ | ||
@disable @implicit this(ref A another) | ||
@implicit this(ref immutable A another) |
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.
missing ;
DIPs/DIP1xxx-rn.md
Outdated
A a = ia; // 1 - calls copy constructor | ||
|
||
B bc; | ||
B b = bc; // 2 - b is evaluated to B.fun |
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.
AFAICT this is a declaration of b, not an "evalution". Did you mean to declare b
of type A
?
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.
Yes.
DIPs/DIP1xxx-rn.md
Outdated
} | ||
``` | ||
|
||
3. When a parameter is returned by value from a function: |
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.
How does calling the copy constructor on the return value interact with RVO (return value optimization)? I guess it might be omitted then.
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.
Yes.
DIPs/DIP1xxx-rn.md
Outdated
``` | ||
|
||
With the current state of the language, `x` is printed once. with the addition of the copy constructor without | ||
`@implicit`, `x` would be printed twice, thus modifying the semantics of existing code. |
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.
Is @implicit
really necessary?
Put other ways:
Is the breakage due to the reservation of @implicit
worse than modifying the semantics of existing code (especially if transitioned with a -dip10xx
flag)?
If someone is declaring a constructor of signature this( ref [qualified] typeof(this) a) [qualified]
, is there any case that they don't want it to be a copy constructor given that the copy constructor is to replace the postblit (again with transition)?
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.
@implicit
doesn’t need to be reserved, it can be a compiler recogized UDA.
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'm saying it doesn't need to exist, i.e. @implicit
can itself, be implicit under a compiler switch.
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.
Honestly, I would expect using @implicit
(or any other attribute) for copy constructors would ultimately cause more bugs than it would ever prevent. Very few functions that would qualify as copy constructors if they were marked with @implicit
will have been declared in D code at this point, because it really doesn't make much sense to (that's what postblit constructors are for), and if they were declared, does anyone honestly expect them to be intended to work as anything other than copy constructors (albeit ones that must currently be called explicitly)? While not requiring @implicit
might break existing code, it's not particularly likely, and long term, it would be far better to have a transitional compiler switch than to require @implicit
just for fear of breaking code that is likely to be extremely rare (if it exists at all) and which likely would work perfectly well with the changes anyway.
On the other hand, if we require @implicit
, then it's bound to become a common bug to forget @implicit
, resulting in code that you think is using a custom copy constructor but in fact is using the default, because you forgot @implicit
. So, we're talking about trying to avoid breaking functions that are likely to be extremely rare by using a solution that's pretty much guaranteed to cause plenty of bugs in the future. I agree that we should be trying to avoid breaking exist code, but that tradeoff is just plain horrible. If we don't want to just immediately, silently break extremely rare code, then providing a compiler switch initially like we have with other DIPs would make a lot more sense. It might even make more sense anyway if we want a chance to bang on the implementation of copy constructors first to make sure that they're working properly before forcing them on everyone.
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.
and if they were declared, does anyone honestly expect them to be intended to work as anything other than copy constructors (albeit ones that must currently be called explicitly)
I think that this is a major point in the need of @implicit. When the user defined the constructor (even though it was intended to work as a copy constructor) he/she didn't have any expectation that it would get called implicitly. The fact that the function was designed to be called explicitly is the reason why @implicit is needed.More than that, there might be users out there that want a copy constructor which is called explicitly and one that is called implicitly (I can't think of any use case, but regardless of that, there might be folks that want it); by not requiring @implicit
there is no way they can achieve it.
On the other hand, if we require @implicit, then it's bound to become a common bug to forget @implicit
This is mere speculation.
So, we're talking about trying to avoid breaking functions that are likely to be extremely rare by using a solution that's pretty much guaranteed to cause plenty of bugs in the future.
When the people will read the changelog and see that the copy constructor feature is now available, I'm guessing that they are going to read the specification first and so copy construction is going to be associated with @implicit
.
Other pros for @implicit:
-> it is a lot easier to spot copy constructors and differentiate them from normal constructors when reading code when you see @implicit
-> it will be easier to extend the feature to copy construct from any type to any type.
-> it brings more clarity (you know that that specific constructor is used implicitly as opposed to an unmarked copy constructor which it's a bit confusing if it is used implicitly or explicitly)
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.
didn't have any expectation that it would get called implicitly.
That is the entire point of the transitional process.
there might be users out there that want a copy constructor which is called explicitly and one that is called implicitly
This is also speculation.
On the other hand, if we require @implicit, then it's bound to become a common bug to forget @implicit
This is mere speculation.
In contrast to all the other attributes they can be ignored and the code is unaffected or the compiler emits an error e.g. I forget nothrow
: either is doesn't matter (because the code that calls it is not nothrow), or it is and the compiler complains. With this I forget
@implicit`, and the code is wrong and still compiles.
it is a lot easier to spot copy constructors and differentiate them from normal constructors when reading code when you see @implicit
Is it really that much worse than for postblits? Is that even a problem?
it will be easier to extend the feature to copy construct from any type to any type.
Is this a thing we are even considering?
it brings more clarity
It does. Is it needed?
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.
This is also speculation.
It is not speculation, it is something you can do by the rules of the language and it will become impossible if @implicit is dropped.
Is this a thing we are even considering?
Yes.
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.
Is this a thing we are even considering?
Yes.
💛 !!!
On @implicit
: I really like the explicit part of being implicit.
But this analysis:
On the other hand, if we require @implicit, then it's bound to become a common bug to forget @implicit, resulting in code that you think is using a custom copy constructor but in fact is using the default, because you forgot @implicit.
I think will be quite true once you add another part of this DIP:
Whenever an initialization by copying is encountered, a call to a copy constructor is tried
and if a matching copy constructor is found it is used, otherwise if no copy constructor is
suitable, standard copying is employed
Experience says that people forget extra configurations, so this will happen if the compiler does the standard copying by default. People will think they've defined a copy ctor.
Is it possible to only perform standard copying iff an implicit copy constructor is actually declared. This way you'll get a compiler error : cannot implicitly copy type, please add an @implicity copy contructor
or something. Is that possible?
Or maybe the postblit makes that a bit difficult :/
I guess a transition flag may help here ;)
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.
It is not speculation,
It is, you missed
there might be users out there that want
(emphasis mine), you also provided no use case for it.
Is this a thing we are even considering?
Yes.
Oh dear (and you thought @implicit
was contentious...) . Regardless, that is not part of this DIP; this DIP is replacing something that is implicit (postblit), with something else that is implicit (copy construction).
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 see how anyone could argue that it's not going to be a common thing to forget to use @implicit
. It doesn't actually add anything. All it does is turn on the behavior that everyone is going to expect. Sure, over time, folks will get used to it and be better at remembering it, but I can guarantee that many folks will forget it just like they forget other attributes. However, in this case, instead of getting a compiler warning because you did something like call an @system
function from an @safe
function, because you forgot to mark the function with @safe
, you'll end up with the default copying mechanism silently being used instead of your custom copy constructor, which may or may not quickly be caught. So, if we ignore the issue of possibly breaking existing code, all @implicit
does is provide us an easy way to have extra bugs.
There really is no reason to be declaring a copy constructor if you don't intend for it to be implicit. So, every copy constructor is going to need the attribute, making it so that it's simply a source of bugs (when it's forgotten) rather than a benefit. The only reason to have @implicit
is to avoid breaking code which is almost certainly extremely rare. We've made breaking changes in the past like this by using compiler switches to enable the transition. Why don't we simply do that here as well? I see no benefit to having @implicit
over a transitional compiler switch and several negatives. Adding @implicit
for fear of breaking rare code just means that we're trying to avoid short term breakage at the cost of making the language worse in the long term - and we already have a way around this that we've used with other DIPS. So, let's please use a transitional compiler switch rather than introducing @implicit
for copy constructors. @implicit
is just going to make the language worse.
struct A { this(this) const {} } | ||
struct B { this(this) immutable {} } | ||
struct C { this(this) shared {} } | ||
``` |
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.
All these produce deprecation errors now.
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.
Yes, I made the PR deprecating them, however a deprecation is not the same as rejecting code (error).
Just copying from what I wrote in the forum:
|
DIPs/DIP1xxx-rn.md
Outdated
C e = a; // error | ||
} | ||
``` | ||
2. It is illegal to declare a copy constructor for a struct that has a postblit defined and vice versa: |
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.
This requirement makes it impossible to transition to this DIP, I suggest a transitional -dip10xx
flag that, if set, will prefer to call a copy constructor over a postblit (probably along with a -vpostblit
to make sure that you're doing what you think you are, especially w.r.t qualifier combinations). It will also make it easier to keep compatibility with older compilers, which is very important.
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.
How come it makes the transition impossible?
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.
Ah, it's because a struct "has" a postblit even if one of its members does and "it" actually does not ... then I see what you mean.
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.
More that that, unless the -dip10xx
flag defines a version (or one defines their own to ease transition) you can't have the two defined which makes it much more difficult to switch between if, for some reason, your code breaks. It also means that you must version your code in order to compile with older compilers.
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.
because a struct "has" a postblit even if one of its members does and "it" actually does not
That doesn't help either.
It is illegal. It is not clearly stated, but I thought it can be interpreted as such. I will add a mention.
The copy constructor may be used in same places as a postblit => unions may not have copy constructors or fields with copy constructors.
The philosophy is that copy constructors are declared whenever the user specifically wants to do something different than standard copying for a specific source-destination qualifier combination. This is a "pay on demand" strategy as you have to define different copy constructors as long as you are interested in doing things differently. If you want to treat mutable-mutable, immutable-immutable, const-const the same, you can just write an
The copy constructor cannot be templated. For now, you can use |
Even with a template |
Yes. |
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.
Awesome stuff! :D
One thing that comes to mind which was not mentioned is a copy constructor with more than one parameter where all except for the first parameter have default arguments. I would think that should be allowed as a valid copy constructor as well, as it allows for one very handy idiom (the FILE, LINE, FUNCTION one).
With the current state of the language, `x` is printed once. with the addition of the copy constructor without | ||
`@implicit`, `x` would be printed twice, thus modifying the semantics of existing code. | ||
|
||
The argument to the copy constructor is passed by reference in order to avoid infinite recursion (passing by |
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.
If it's only by ref then how does this work:
A rvalue() {
return A();
}
A a = rvalue();
Would DIP 1016 be a prereq for this DIP?
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.
It would be moved, not copied, i think.
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.
Indeed, it is moved; it is stated a bit later that copy construction is performed only if the source is an lvalue.
DIPs/DIP1xxx-rn.md
Outdated
C e = a; // error | ||
} | ||
``` | ||
2. It is illegal to declare a copy constructor for a struct that has a postblit defined and vice versa: |
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.
How come it makes the transition impossible?
DIPs/DIP1xxx-rn.md
Outdated
C e = a; // error | ||
} | ||
``` | ||
2. It is illegal to declare a copy constructor for a struct that has a postblit defined and vice versa: |
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.
Ah, it's because a struct "has" a postblit even if one of its members does and "it" actually does not ... then I see what you mean.
DIPs/DIP1xxx-rn.md
Outdated
```d | ||
struct A | ||
{ | ||
@implicit this(ref A another) {} // 1 - mutable source, mutable destination |
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.
If it was not wanted then wouldn't the author make another
a const ref
or?
DIPs/DIP1xxx-rn.md
Outdated
|
||
When a copy constructor is not defined for a struct, initializations are treated | ||
by copying the contents from the memory location of the right-hand side expression | ||
to the memory location of the left-hand side one (called "standard copying"): |
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.
Isn't this already called "blitting"?
DIPs/DIP1xxx-rn.md
Outdated
For a function in an overload set each argument is matched against the parameter the following way: | ||
|
||
1. Check if the argument type is a direct match to the parameter type. | ||
2. If not, check if the argument type is implicitly convertible to the parameter type. |
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.
By implicitly convertible it means specifically the qualifiers of the type right? Because they have to be the same type anyway and I assume this doesn't have anything to do with alias this
. In which case the phrasing here makes it seem like two different types are being taken in to consideration.
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.
By implicitly convertible it means specifically the qualifiers of the type right?
In the case of structs, yes, but for classes, implicit conversion is something different. These first 2 steps here were implemented in the compiler before copy constructors, so I choose this phrasing to show what copy constructors bring new into the scheme (which is the 3rd step).
In which case the phrasing here makes it seem like two different types are being taken in to consideration.
immutable T
is different from T
.
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.
Ah, I think I misunderstood the general idea here. Thanks for clarifying!
DIPs/DIP1xxx-rn.md
Outdated
to solve the assignment (1), the copy constructor takes precedence over `alias this` | ||
because it is considered more specialized (the sole purpose of the copy constructor is to | ||
create copies). However, if no copy constructor in the overload set matches the exact | ||
qualified types of the source and the destination, the `alias this` is preferred (2). |
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.
This seems to conflict with (from section copy constructor call vs standard copying)
Whenever an initialization by copying is encountered, a call to a copy constructor is tried
and if a matching copy constructor is found it is used, otherwise if no copy constructor is
suitable, standard copying is employed
- When the overload set does not match, then a standard copy is performed.
- When the overload set does not match and alias this is present, alias this is performed
- A copy constructor is considered to be more specialized
Today, if I have an alias this
and no post blit, the alias this
does not take precedence:
struct A {
int a;
immutable(A) fun() {
writeln("Yo!");
return immutable A(7);
}
alias fun this;
}
void main() {
A a;
A b = a; // No Yo!
}
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.
In your example the alias this is never attempted because there are no undefined lookups (A b = a
is something the compiler knows how to do). However, here is a slightly modified version of your code:
struct A
{
int[] a;
immutable(A) fun()
{
writeln("Yo");
return immutable A([7]);
}
alias fun this;
}
void main()
{
A a;
immutable A b = a; // error: cannot implicitly convert expression a of type A to immutable(A)
}
If I'm not missing something this code should be forwarded to alias this, but it isn't, which IMO is a bug. The section about alias this was trying to state that in such situations copy construction takes precedence over alias this.
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 future reference this bug was fixed).
DIPs/DIP1xxx-rn.md
Outdated
|
||
Inside the body of the generated copy constructors, for the fields that define a copy | ||
constructor, the assignment will be rewritten to a call to it; for those that do not, | ||
standard copying is employed, if possible. If certain fields cannot perform copies |
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 alias this
caveat from above throws a wrench in to this as well no? i.e. for those that do not, standard copying is employed, if possible, unless an alias this assignment is possible
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.
Indeed.
DIPs/DIP1xxx-rn.md
Outdated
``` | ||
|
||
A solution to this might be to make the reference `const`, but that would make code like | ||
`this.a = another.a` inside the copy constructor illegal. Of course, this can be solved by means |
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.
s/would/"could possibly"
depends on the types in A (i.e. if it was a type with no indirections then we all good with a const ref).
Is this in the correct section? This is not a breaking change or a deprecation is it?
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.
If you take into account that the postblit does not offer the possibility of modifying the source, it can be seen as a breaking change. I'm sure that some folks might see it as a feature :D 🗡️
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.
But implicit copy construction was not even possible before so people will have to write new code to do this. Implicit copy construction that may modify the source will not occur ... erm ... "implicitly".
DIPs/DIP1xxx-rn.md
Outdated
|
||
3. With this DIP `@implicit` becomes a compiler recognised attribute that can be used solely to | ||
distinguish copy constructors from normal constructors. This will break code that used `@implicits` | ||
as a used defined attribute: |
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 believe @jacob-carlborg has mentioned somewhere that you can avoid this? If that's possible then that'd be a big plus IMO.
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 honestly think that people should be discouraged to use UDAs that are also special keywords in the compiler.
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.
It's possible, that's how the @selector
attribute is implemented. I don't see a reason making it a keyword.
C++ has efficient for reference counting move constructor (it is a general purpose, but quite good for reference counting). Can we do the same with this DIP? |
It is still missing a transitional |
DIPs/DIP1xxx-rn.md
Outdated
In order to assure a smooth transition from postblit to copy constructor, the following | ||
strategy is employed: if a `struct` defines a postblit (user-defined or generated) all | ||
copy constructor definitions will be ignored for that particular `struct` and the postblit | ||
is going to be used. Existing codebases that didn't use the postblit may start using the |
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.
Minor editorial nit: should probably read "...and the postblit will be used."
implement. | ||
|
||
#### Shared postblits | ||
|
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 whole rationale behind shared postblits
and/or copy constructors is based on assumptions that are, at least in my opinion, misguided. What, exactly, would be a purpose of a shared
copy of a shared
object? shared
types, by definition, cannot, and shall not, be copyable. They're shared unique instances. A shared
struct is either a global, or an allocated instance passed by reference. If you can freely make a copy, then sharing makes absolutely no sense.
The only contexts where it would make sense is acquiring an un-shared
copy from a shared
instance, or instantiating a shared
instance from an un-shared
one, both are beyond simple copying, they're type conversions, procedures which in the general case can only be reasoned about by user code.
Therefore, copy constructors/postblits for shared
types are best made not allowed by the language wholesale, leaving any special treatment (i.e. conversion to/from shared
) up to user code. This particular part of the DIP is trying to reason about a problem that is ill-treated (poorly specified) by the language, and ultimately would become a wasted effort.
@RazvanN7 Hey! I'm going to do my customary editing pass over this on Wednesday or Thursday of this week in preparation for the Community Review. If you've got any outstanding changes to make, please try to get them done before then. Thanks! |
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.
As soon as you've responded to my suggestions, I'll get this into Community Review. Thanks!
DIPs/DIP1xxx-rn.md
Outdated
|
||
## Abstract | ||
|
||
This document proposes the copy constructor semantics as an alternative |
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 copy constructor -> copy constructor
DIPs/DIP1xxx-rn.md
Outdated
|
||
## Rationale and Motivation | ||
|
||
This section highlights the existing problems with the postblit and motivates |
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.
motivates -> demonstrates
DIPs/DIP1xxx-rn.md
Outdated
|
||
This section highlights the existing problems with the postblit and motivates | ||
why the implementation of a copy constructor is more desirable than an attempt | ||
to fix all the postblit issues. |
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.
to fix -> to resolve
DIPs/DIP1xxx-rn.md
Outdated
struct C { this(this) shared {} } | ||
``` | ||
|
||
Since the semantics of the postblit in the presence of qualifiers was |
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.
was -> is
DIPs/DIP1xxx-rn.md
Outdated
``` | ||
|
||
Since the semantics of the postblit in the presence of qualifiers was | ||
not defined and most likely not intended, this led to a series of problems: |
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.
this led to a series of problems -> a series of problems has arisen
DIPs/DIP1xxx-rn.md
Outdated
``` | ||
|
||
A solution to this might be to make the reference `const`, but that would make code like | ||
`this.a = another.a` inside the copy constructor illegal. Of course, this can be solved by means |
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.
Of course, this -> This
DIPs/DIP1xxx-rn.md
Outdated
|
||
A solution to this might be to make the reference `const`, but that would make code like | ||
`this.a = another.a` inside the copy constructor illegal. Of course, this can be solved by means | ||
of casting : `this.a = cast(int[])another.a`. |
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.
by means of casting : -> with a cast, e.g.
DIPs/DIP1xxx-rn.md
Outdated
of casting : `this.a = cast(int[])another.a`. | ||
|
||
2. This DIP changes the semantic of constructors which receive a parameter by reference of type | ||
`typeof(this)`. The consequence is that the constructor might get called implicitly in some |
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 constructor -> existing constructors
might get -> might be
DIPs/DIP1xxx-rn.md
Outdated
} | ||
``` | ||
|
||
Before this DIP this code would not print anything, while after it will print "Yo". However, a case can be |
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.
This will print "Yo" subsequent to the implementation of this DIP, where before it printed nothing.
However, a case -> A case
@mdparker Done. Thank you! |
62a883e
to
796930b
Compare
DIPs/DIP1xxx-rn.md
Outdated
A declaration is a copy constructor declaration if it is a constructor declaration that takes only one | ||
parameter by reference that is of the same type as `typeof(this)`. Declaring a copy constructor in this | ||
manner has the advantage that no parser modifications are required, thus leaving the language grammar | ||
unchanged. |
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.
Do you want to allow additional optional parameters? Like this(ref typeof(this) x, bool deepCopy = false)
. C++ allows it. You might want to mention it, either in support or rejection.
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.
Yah. Thanks! To answer your question: default parameters will be supported.
A b; | ||
b = gun(); // NRVO cannot be performed, copy constructor is called | ||
} | ||
``` |
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.
Isn't the copy done at the returns? That way, fun
does not need to copy a
, it basically moves its local value, whereas gun
has to copy the value to return it. The copy is them moved in the assignment b = gun()
. NRVO is an optimization and not part of semantics.
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.
That is true, but the effect is as if the copy constructor would get called on the returned value. I wanted to avoid going into compiler specific generated code such as (A __tmp; __tmp.copyCtor(a); __tmp)
for the return a
line in the gun function.
|
||
In order to disable copy construction, all copy constructor overloads must be disabled. | ||
In the above example, only copies from mutable to mutable are disabled; the overload for | ||
immutable to mutable copies is still callable. |
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.
Later you define implicitly generated copy constructor overloads. Do they have to be disabled manually or do you mean all explicitly written copy constructor overloads?
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 meant explicitly, in order to disable a generated copy constructor one must disable the copy constructor of the member. Anyway, after a chat with Walter we decided to give up on generating copy constructors in the way currently mentioned. I will update the DIP with the new strategy, soon.
|
||
When a copy constructor is not defined for a `struct`, initializations are handled | ||
by copying the contents from the memory location of the right-hand side expression | ||
to the memory location of the left-hand side expression (i.e. "blitting"): |
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.
Maybe use the wording: "... as if it was implemented using [the workaround above]". I think, this wording is too much, what the compiler should do and not what should happen.
DIPs/DIP1xxx-rn.md
Outdated
B bc; | ||
B b = bc; // 2 - calls B.fun | ||
} | ||
``` |
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.
There is something weird. Why should B b = bc;
use alias this? The copy constructor can take bc
perfectly. On the other hand, A a = ia;
won't compile. The copy constructor cannot take an immutable value and A.fun
cannot be used on an immutable value.
Also better rename the fields a
of A
and B
to something different like n
. It would make discussing the code easier.
DIPs/DIP1xxx-rn.md
Outdated
constructor will be called (if possible) when the source is passed as a parameter to the `opAssign` | ||
function. | ||
|
||
### POD (Plain Old Data) |
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.
Better use "Plain Old Data" as the header. Introduce the abbreviation in the next sentence.
DIPs/DIP1xxx-rn.md
Outdated
|
||
2. The copy constructor signature is: `this(ref $q1 S rhs) $q2`, where `q1` and `q2` represent the | ||
qualifiers that can be applied to the function and its parameter (`const`, `immutable`, `shared`, etc.). | ||
Depending on the values of `$q1` and `$q2`, what should the signature of `opAssign` be? |
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.
Maybe use qual1
instead of $q1
. This is not D syntax; introduce qual1
before using it. Like:
"Using qual_1
and qual_2
for any of the qualifiers that can be applied to the function and its parameter, ...". Names matter.
DIPs/DIP1xxx-rn.md
Outdated
the sake of simplicity, `opAssign` will be generated solely for copy constructors that have a missing `$q2`. | ||
|
||
Taking into account the above restrictions, we can generate a single `opAssign` function | ||
and leverage the existing `opAssign` generation logic for the [[postblit](https://github.com/dlang/dmd/pull/8505)]. |
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.
Dangling bracket here: [[postblit]
DIPs/DIP1xxx-rn.md
Outdated
A solution might be to generate the counterpart `opAssign` for each copy constructor, e.g. `void opAssign(ref $q1 S rhs) $q2`. | ||
However, when is a `const`/`immutable` `opAssign` needed? There might be obscure cases when it is useful, but those are | ||
niche situations where the user must step in to clarify the desired outcome, and define its own `opAssign`. For | ||
the sake of simplicity, `opAssign` will be generated solely for copy constructors that have a missing `$q2`. |
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.
s/missing/mutable or "for copy constructors that create mutable values"
`this.a = another.a` inside the copy constructor illegal. This can be solved with a cast, e.g. | ||
`this.a = cast(int[])another.a`. | ||
|
||
2. This DIP changes the semantic of constructors which receive a parameter by reference of type |
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.
s/semantic/semantics (see here)
cf08621
to
efacbf8
Compare
No description provided.