-
-
Notifications
You must be signed in to change notification settings - Fork 755
std.conv.emplace fixes & improvements
#896
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
Changes from all commits
6147510
d207cd0
6639900
e62b241
6350320
98061d2
524fe42
4bc2194
dab3b2a
9cf123a
2840e44
10375d6
ca9ef19
4269dd5
b3d3c44
deb33d0
3e0f69c
95c5ba3
d038acb
0511b07
525fb51
603d2e2
c31fcdc
44ac2b7
4431001
e541499
1450381
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3405,10 +3405,9 @@ as $(D chunk)). | |
| T* emplace(T)(T* chunk) | ||
| if (!is(T == class)) | ||
| { | ||
| auto result = cast(typeof(return)) chunk; | ||
| static T i; | ||
| memcpy(result, &i, T.sizeof); | ||
| return result; | ||
| static T i; // Can't use `= T.init` here because of @@@BUG8902@@@. | ||
| memcpy(chunk, &i, T.sizeof); | ||
| return chunk; | ||
| } | ||
| ///ditto | ||
| T* emplace(T)(T* chunk) | ||
|
|
@@ -3418,6 +3417,64 @@ T* emplace(T)(T* chunk) | |
| return chunk; | ||
| } | ||
|
|
||
| version(unittest) private struct __conv_EmplaceTest | ||
| { | ||
| int i = 3; | ||
| this(int i) | ||
| { | ||
| assert(this.i == 3 && i == 5); | ||
| this.i = i; | ||
| } | ||
| this(int i, ref int j) | ||
| { | ||
| assert(i == 5 && j == 6); | ||
| this.i = i; | ||
| ++j; | ||
| } | ||
|
|
||
| @disable: | ||
| this(); | ||
| this(this); | ||
| void opAssign(); | ||
| } | ||
|
|
||
| version(unittest) private class __conv_EmplaceTestClass | ||
| { | ||
| int i = 3; | ||
| this(int i) | ||
| { | ||
| assert(this.i == 3 && i == 5); | ||
| this.i = i; | ||
| } | ||
| this(int i, ref int j) | ||
| { | ||
| assert(i == 5 && j == 6); | ||
| this.i = i; | ||
| ++j; | ||
| } | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| struct S { @disable this(); } | ||
| S s = void; | ||
| static assert(!__traits(compiles, emplace(&s))); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| interface I {} | ||
| class K : I {} | ||
|
|
||
| K k = void; | ||
| emplace(&k); | ||
| assert(k is null); | ||
|
|
||
| I i = void; | ||
| emplace(&i); | ||
| assert(i is null); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| Given a pointer $(D chunk) to uninitialized memory (but already typed | ||
|
|
@@ -3437,39 +3494,144 @@ T* emplace(T, Args...)(T* chunk, Args args) | |
| return chunk; | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| debug(conv) scope(success) writeln("unittest @", __FILE__, ":", __LINE__, " succeeded."); | ||
| int a; | ||
| int b = 42; | ||
| assert(*emplace!int(&a, b) == 42); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| interface I {} | ||
| class K : I {} | ||
|
|
||
| K k = null, k2 = new K; | ||
| assert(k !is k2); | ||
| emplace!K(&k, k2); | ||
| assert(k is k2); | ||
|
|
||
| I i = null; | ||
| assert(i !is k); | ||
| emplace!I(&i, k); | ||
| assert(i is k); | ||
| } | ||
|
|
||
| // Specialization for struct | ||
| T* emplace(T, Args...)(T* chunk, Args args) | ||
| T* emplace(T, Args...)(T* chunk, auto ref Args args) | ||
| if (is(T == struct)) | ||
| { | ||
| auto result = cast(typeof(return)) chunk; | ||
|
|
||
| void initialize() | ||
| { | ||
| static T i; | ||
| memcpy(chunk, &i, T.sizeof); | ||
| if(auto p = typeid(T).init().ptr) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should look at the generated code and see whether this is slower than the previous code (I don't think so; at best it would be as fast). So we need to keep the old code, but with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only 'bad' thing here is the little indirection it takes to get to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth noting that array copy syntax could be used here, though: As far as I know,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what advantage does this hold over the memcpy thing? That seems the most direct to me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It compiles with current dmd.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is correct. It's not null. )))
No it can't. You have to remember this hard as it's tricky: struct S
{ int i; }
void main()
{
// yes, all asserts passes
auto init = typeid(S).init();
assert(init !is null);
assert(init.ptr is null);
assert(init.length is 4);
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. But there are several places in druntime in particular where the array copy syntax is used to initialize objects manually like that, and they don't seem to have a problem with that. So what gives?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As I see, druntime always uses And according to dmd ABI the first class field is
E.g. ability to call |
||
| memcpy(chunk, p, T.sizeof); | ||
| else | ||
| memset(chunk, 0, T.sizeof); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange, I think I misplaced my above comment. It was meant for here. Got angry when my browser lost it and had to re-type everything >:( #8902 is indeed strange. How about something like this then? static if (is(typeof((inout int _dummy=0){static T i;})))
{
static T i;
memcpy(chunk, &i, T.sizeof);
}
else //@@@BUG@@@ 8902
{
if(auto p = typeid(T).init().ptr)
memcpy(chunk, p, T.sizeof);
else
memset(chunk, 0, T.sizeof);
}It avoids paying for the general case, while still supporting the odd case. I would have wished I was able to test
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you call static T i = T.init; not just And I'm still not sure about the second branch name because I'm not an expert in {[copy ]con|de}structors, initialization, and current compiler bugs in this area. And
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but apparently, the "Union U" case passes the static if, and then ends up in branch 1, and then fails :/ It is (IMO) VERY strange. I documented it in the bug report, and elevated it because of this.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey wait, you are right: This is the struct implementation, so enums should not be an issue . Using:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Did you mean unions instead of enums? You are missing this case: union U { int a, b; }
struct S { U u; }
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, and Yes. :( What a pain!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, |
||
| } | ||
|
|
||
| static if (is(typeof(result.__ctor(args)))) | ||
| static if (is(typeof(chunk.__ctor(args)))) | ||
| { | ||
| // T defines a genuine constructor accepting args | ||
| // Go the classic route: write .init first, then call ctor | ||
| initialize(); | ||
| result.__ctor(args); | ||
| chunk.__ctor(args); | ||
| } | ||
| else static if (is(typeof(T(args)))) | ||
| { | ||
| // Struct without constructor that has one matching field for | ||
| // each argument | ||
| *result = T(args); | ||
| *chunk = T(args); | ||
| } | ||
| else //static if (Args.length == 1 && is(Args[0] : T)) | ||
| { | ||
| static assert(Args.length == 1); | ||
| //static assert(0, T.stringof ~ " " ~ Args.stringof); | ||
| // initialize(); | ||
| *result = args[0]; | ||
| *chunk = args[0]; | ||
| } | ||
| return result; | ||
| return chunk; | ||
| } | ||
|
|
||
| // Test constructor branch | ||
|
|
||
| unittest | ||
| { | ||
| debug(conv) scope(success) writeln("unittest @", __FILE__, ":", __LINE__, " succeeded."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prolly we can delete this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but in a separate pull. Note that this pulls contains only obvious fixes for most nasty bugs of |
||
| struct S | ||
| { | ||
| double x = 5, y = 6; | ||
| this(int a, int b) | ||
| { | ||
| assert(x == 5 && y == 6); | ||
| x = a; | ||
| y = b; | ||
| } | ||
| } | ||
|
|
||
| auto s1 = new void[S.sizeof]; | ||
| auto s2 = S(42, 43); | ||
| assert(*emplace!S(cast(S*) s1.ptr, s2) == s2); | ||
| assert(*emplace!S(cast(S*) s1, 44, 45) == S(44, 45)); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| __conv_EmplaceTest k = void; | ||
| emplace(&k, 5); | ||
| assert(k.i == 5); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| int var = 6; | ||
| __conv_EmplaceTest k = void; | ||
| emplace(&k, 5, var); | ||
| assert(k.i == 5); | ||
| assert(var == 7); | ||
| } | ||
|
|
||
| // Test matching fields branch | ||
|
|
||
| unittest | ||
| { | ||
| struct S { uint n; } | ||
| S s; | ||
| emplace!S(&s, 2U); | ||
| assert(s.n == 2); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| struct S { int a, b; this(int){} } | ||
| S s; | ||
| static assert(!__traits(compiles, emplace!S(&s, 2, 3))); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| struct S { int a, b = 7; } | ||
| S s1 = void, s2 = void; | ||
|
|
||
| emplace!S(&s1, 2); | ||
| assert(s1.a == 2 && s1.b == 7); | ||
|
|
||
| emplace!S(&s2, 2, 3); | ||
| assert(s2.a == 2 && s2.b == 3); | ||
| } | ||
|
|
||
| // Test assignment branch | ||
|
|
||
| // FIXME: no tests | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me in my incoming pull from my self-explaining |
||
|
|
||
| private void testEmplaceChunk(void[] chunk, size_t typeSize, size_t typeAlignment, string typeName) | ||
| { | ||
| enforceEx!ConvException(chunk.length >= typeSize, | ||
| xformat("emplace: Chunk size too small: %s < %s size = %s", | ||
| chunk.length, typeName, typeSize)); | ||
| enforceEx!ConvException((cast(size_t) chunk.ptr) % typeAlignment == 0, | ||
| xformat("emplace: Misaligned memory block (0x%X): it must be %s-byte aligned for type %s", | ||
| chunk.ptr, typeAlignment, typeName)); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -3485,14 +3647,11 @@ $(D T) is $(D @safe). | |
|
|
||
| Returns: A pointer to the newly constructed object. | ||
| */ | ||
| T emplace(T, Args...)(void[] chunk, Args args) if (is(T == class)) | ||
| T emplace(T, Args...)(void[] chunk, auto ref Args args) if (is(T == class)) | ||
| { | ||
| enum classSize = __traits(classInstanceSize, T); | ||
| enforce(chunk.length >= classSize, | ||
| new ConvException("emplace: chunk size too small")); | ||
| auto a = cast(size_t) chunk.ptr; | ||
| enforce(a % T.alignof == 0, text(a, " vs. ", T.alignof)); | ||
| auto result = cast(typeof(return)) chunk.ptr; | ||
| testEmplaceChunk(chunk, classSize, classInstanceAlignment!T, T.stringof); | ||
| auto result = cast(T) chunk.ptr; | ||
|
|
||
| // Initialize the object in its pre-ctor state | ||
| (cast(byte[]) chunk)[0 .. classSize] = typeid(T).init[]; | ||
|
|
@@ -3513,6 +3672,14 @@ T emplace(T, Args...)(void[] chunk, Args args) if (is(T == class)) | |
| return result; | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| int var = 6; | ||
| auto k = emplace!__conv_EmplaceTestClass(new void[__traits(classInstanceSize, __conv_EmplaceTestClass)], 5, var); | ||
| assert(k.i == 5); | ||
| assert(var == 7); | ||
| } | ||
|
|
||
| /** | ||
| Given a raw memory area $(D chunk), constructs an object of non-$(D | ||
| class) type $(D T) at that address. The constructor is passed the | ||
|
|
@@ -3525,15 +3692,11 @@ $(D T) is $(D @safe). | |
|
|
||
| Returns: A pointer to the newly constructed object. | ||
| */ | ||
| T* emplace(T, Args...)(void[] chunk, Args args) | ||
| T* emplace(T, Args...)(void[] chunk, auto ref Args args) | ||
| if (!is(T == class)) | ||
| { | ||
| enforce(chunk.length >= T.sizeof, | ||
| new ConvException("emplace: chunk size too small")); | ||
| auto a = cast(size_t) chunk.ptr; | ||
| enforce(a % T.alignof == 0, text(a, " vs. ", T.alignof)); | ||
| auto result = cast(typeof(return)) chunk.ptr; | ||
| return emplace(result, args); | ||
| testEmplaceChunk(chunk, T.sizeof, T.alignof, T.stringof); | ||
| return emplace(cast(T*) chunk.ptr, args); | ||
| } | ||
|
|
||
| unittest | ||
|
|
@@ -3552,26 +3715,10 @@ unittest | |
|
|
||
| unittest | ||
| { | ||
| debug(conv) scope(success) writeln("unittest @", __FILE__, ":", __LINE__, " succeeded."); | ||
| int a; | ||
| int b = 42; | ||
| assert(*emplace!int(&a, b) == 42); | ||
|
|
||
| struct S | ||
| { | ||
| double x = 5, y = 6; | ||
| this(int a, int b) | ||
| { | ||
| assert(x == 5 && y == 6); | ||
| x = a; | ||
| y = b; | ||
| } | ||
| } | ||
|
|
||
| auto s1 = new void[S.sizeof]; | ||
| auto s2 = S(42, 43); | ||
| assert(*emplace!S(cast(S*) s1.ptr, s2) == s2); | ||
| assert(*emplace!S(cast(S*) s1, 44, 45) == S(44, 45)); | ||
| int var = 6; | ||
| auto k = emplace!__conv_EmplaceTest(new void[__conv_EmplaceTest.sizeof], 5, var); | ||
| assert(k.i == 5); | ||
| assert(var == 7); | ||
| } | ||
|
|
||
| unittest | ||
|
|
@@ -3610,38 +3757,6 @@ unittest | |
| assert(equal(map!(to!int)(["42", "34", "345"]), [42, 34, 345])); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| struct Foo | ||
| { | ||
| uint num; | ||
| } | ||
|
|
||
| Foo foo; | ||
| emplace!Foo(&foo, 2U); | ||
| assert(foo.num == 2); | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| interface I {} | ||
| class K : I {} | ||
|
|
||
| K k = void; | ||
| emplace!K(&k); | ||
| assert(k is null); | ||
| K k2 = new K; | ||
| assert(k2 !is null); | ||
| emplace!K(&k, k2); | ||
| assert(k is k2); | ||
|
|
||
| I i = void; | ||
| emplace!I(&i); | ||
| assert(i is null); | ||
| emplace!I(&i, k); | ||
| assert(i is k); | ||
| } | ||
|
|
||
| // Undocumented for the time being | ||
| void toTextRange(T, W)(T value, W writer) | ||
| if (isIntegral!T && isOutputRange!(W, char)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2781,6 +2781,43 @@ unittest | |
| } | ||
|
|
||
|
|
||
| private template maxAlignment(U...) if(isTypeTuple!U) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An emerging idiom has it that templates that compute values are best expressed as CTFE-able functions whereas templates that deal only in types are best expressed as templates.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do you see that idiom emerging? I always prefer templates to CTFE functions with only template parameters, and I wouldn't have noticed so far that Phobos had a different policy. Besides, Denis just moved that template over from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah, that's a relatively new thing. At this point it's unclear what's the best policy to decide between CTFE and sheer templates. Ideally whatever computes values would be CTFE.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @klickverbot, yes templates are great! |
||
| { | ||
| static if(U.length == 1) | ||
| enum maxAlignment = U[0].alignof; | ||
| else | ||
| enum maxAlignment = max(U[0].alignof, .maxAlignment!(U[1 .. $])); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| Returns class instance alignment. | ||
|
|
||
| Example: | ||
| --- | ||
| class A { byte b; } | ||
| class B { long l; } | ||
|
|
||
| // As class instance always has a hidden pointer | ||
| static assert(classInstanceAlignment!A == (void*).alignof); | ||
| static assert(classInstanceAlignment!B == long.alignof); | ||
| --- | ||
| */ | ||
| template classInstanceAlignment(T) if(is(T == class)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually it's more comfy to call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I like them too. The matter here is, as the complexity of the computation goes up, so does the awkwardness of doing it in a template as opposed to a function. So at a point it's better to relegate computations to CTFEs. But using difficulty of implementation as a decision for using template vs. using CTFE is too subjective (besides what does client code care about how hard it was for the implementer?) so that's what makes it attractive to just say: if it's computation use CTFE, otherwise use template. |
||
| { | ||
| alias maxAlignment!(void*, typeof(T.tupleof)) classInstanceAlignment; | ||
| } | ||
|
|
||
| unittest | ||
| { | ||
| class A { byte b; } | ||
| class B { long l; } | ||
|
|
||
| static assert(classInstanceAlignment!A == (void*).alignof); | ||
| static assert(classInstanceAlignment!B == long.alignof); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems wrong to me; it should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But perhaps you intentionally made it work this way to always pick the largest possible alignment on any system? If so, should document that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. largest alignment would prolly be real.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't align a pointer on an 8-byte boundary on a 32-bit system; you align it on a 4-byte boundary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But there is a |
||
| } | ||
|
|
||
|
|
||
| //:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::// | ||
| // Type Conversion | ||
| //:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::// | ||
|
|
||
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 a bit concerned about this approach. Is allocating a TypeInfo object really necessary? Not only does it incur a run-time cost, it also means emplace is now never nothrow nor safe (I think right now, it could at least be implicitly nothrow...)
Why not just keep it as is, but while simply adding T.init initialization?
AFAIK, the only object for which this would not work, are objects that wouldn't be emplaceable anyways, such as immutables. Am I missing any other types?
Worst case scenario, wouldn't it be a better approach to fallback to your mechanism, only when the simpler T.init approach fails? At least this way the allocation only occurs when it is justified...
What do you 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.
For now it doesn't work at least because of 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.
But yes, it's strange. Filled issue Issue 8902 - Unexpected "duplicate union initialization for X" 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.
Anyhow,
static T i;should be equivalent tostatic T i = T.init;. Better yet, let's make thisstatic immutable T i;so as to avoid thread-local storage.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. The first one will fail for
@disabled this().Same problem with
@disabled this()forimmutable T i;And Issue 8902 forstatic immutable S s = S.init;.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.
@denis-sh: So maybe adding a comment in the source there might be a good idea? ;)
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 not asking it again. Judging by the discussion, the
= T.initshould be there.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 it will not compile because of Issue 8902. Are you judged me to fix a 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.
Great, thanks. The point here is that this portion of code must be carefully documented as it's clearly non-intuitive and contingent (and dependent) on certain idiosyncrasies. Pulls come and go, but comments stay with the code. So please let's make sure the choices here are explained in comments, including an explanation on why the "obvious" choices won't work. 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.
Done.