Fix Issue 19022 - CTorFlow: Show the line of the duplicated initialization for const/immutable fields #8399
Conversation
|
Thanks for your pull request, @wilzbach! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#8399" |
wilzbach
changed the title
src/dmd/root/rmem.d
Outdated
|
|
||
| Returns: A copy of the input array. | ||
| */ | ||
| extern (D) static auto arraydup(T)(T[] s) nothrow |
There was a problem hiding this comment.
staticis not necessary- please don’t use
autofor the return type - please use standard D naming conventions
- can this be made
pure?
There was a problem hiding this comment.
FYI: I copied the declaration of the previous xarraydup, hence the name.
can this be made pure?
Yes, if the pureMalloc declarations are added. However, I don't see a big to do this in this PR as xarraydup isn't pure either, DMD can't take much advantage of purity anyhow and this PR still has other, real issues.
src/dmd/root/rmem.d
Outdated
|
|
||
| Returns: A copy of the input array. | ||
| */ | ||
| extern (D) static auto arraydup(T)(T[] s) nothrow |
There was a problem hiding this comment.
staticis not necessary- please don’t use
autofor the return type - please use standard D naming conventions
- can this be made
pure?
src/dmd/root/rmem.d
Outdated
| */ | ||
| extern (D) static auto arraydup(T)(T[] s) nothrow | ||
| { | ||
| if (s !is null) |
There was a problem hiding this comment.
I suggest an early return.
wilzbach
force-pushed
the
fix-19022
branch
3 times, most recently
from
25e7d34
to
448f3b6
Compare
|
Instead of using to separate arrays, I now merged them (CSX + Location info) into one. |
wilzbach
changed the title
test/fail_compilation/fail9665a.d
Outdated
| fail_compilation/fail9665a.d(45): Error: immutable field `v` initialized multiple times | ||
| fail_compilation/fail9665a.d(44): Previous initialization is here. | ||
| fail_compilation/fail9665a.d(55): Error: immutable field `v` initialized multiple times | ||
| Previous initialization is here. |
There was a problem hiding this comment.
This is missing file and line information as far as I can see.
wilzbach
force-pushed
the
fix-19022
branch
2 times, most recently
from
9b5045f
to
24e4a0c
Compare
wilzbach
force-pushed
the
fix-19022
branch
3 times, most recently
from
dbbd592
to
e7cbc9d
Compare
…ation for const/immutable fields
| @@ -127,9 +129,14 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1) | |||
| // dmd.ctorflow.CSX enum. | |||
| // @@@DEPRECATED_2019-01@@@. | |||
| if (fi & CSX.deprecate_18719) | |||
| .deprecation(loc, "%s field `%s` initialized multiple times", modStr, var.toChars()); | |||
| { | |||
| .deprecation(loc, "%s field `%s` was initialized in a previous constructor call", modStr, var.toChars()); | |||
There was a problem hiding this comment.
The fix for 18719 is too aggressive and only checks whether a this call is present.
Hence, there's no way to get the line number of the field with the previous initialization (there might be none after all).
See also: https://issues.dlang.org/show_bug.cgi?id=19030
There was a problem hiding this comment.
Ok. Perhaps an intermediate solution can be implemented. For example, printing the line number of the constructor call if there is no line number for the field.
There was a problem hiding this comment.
The problem is the as of now we don't have this information. It currently triggers this deprecation whenever a this(...) call has been observed and a immutable field is modified. This means if there are three constructors it could be any of the two other.
I wasn't sure on the best way to save this information.
Is it okay to use an additional array here?