Fix Issue 10879 - std.variant Variant/Algebraic: Cant store static ar… by Biotronic · Pull Request #4204 · dlang/phobos

Biotronic · 2016-04-16T13:38:09Z

…rays > 32(/16) bytes

dlang-bot · 2016-04-16T13:38:12Z

Thanks for your pull request, @Biotronic! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	10879	std.variant Variant/Algebraic: Can't store static arrays > 32(/16) bytes

DmitryOlshansky · 2016-04-16T15:42:58Z

std/variant.d

            return !tryPutting(null, *cast(TypeInfo*) parm, null);
        case OpID.compare:
        case OpID.equals:
+            import std.stdio;


Debugging leftover?

I have no idea what you're talking about.

Biotronic · 2016-04-17T20:52:58Z

It could be argued that this bug should be fixed in DMD rather than Phobos, in that 'new T' should work for any T, even something like int[10].

DmitryOlshansky · 2016-04-20T10:23:19Z

LGTM though better fix DMD

Biotronic · 2016-04-20T10:42:21Z

I made a fix in DMD as well. If dlang/dmd#5688 is accepted, this pull can be ignored.

wilzbach · 2016-05-10T21:08:28Z

LGTM though better fix DMD

can we have one of those "decision blocked" labels? thanks!

tramker · 2016-11-06T01:22:01Z

Anything new ? Since the dmd pull has stalled, maybe this can be pulled ?

MetaLang · 2017-07-21T18:58:02Z

@tramker sorry this got stalled. If you rebase onto the latest version of master I will review it.

MetaLang

I really wish Github would notify me when a PR I've commented on is rebased.

MetaLang · 2017-08-06T18:21:35Z

std/variant.d

+    v2 = arr;
+    assert(v1 == arr);
+    assert(v2 == arr);
+    foreach (i, e; arr) {


Please put braces on their own line.

MetaLang · 2017-08-06T18:21:43Z

std/variant.d

+        assert(v1[i] == e);
+        assert(v2[i] == e);
+    }
+    static struct LargeStruct {


MetaLang · 2017-08-06T18:37:09Z

std/variant.d

+                {
+                    static if (is(A a == U[n], U, size_t n))
+                    {
+                        pragma(msg, "A");


Please remove this.

MetaLang · 2017-08-06T18:37:17Z

std/variant.d

+                    }
+                    else
+                    {
+                        pragma(msg, "B");


MetaLang · 2017-08-06T18:40:26Z

std/variant.d

                if (target.type.tsize < A.sizeof)
-                    *cast(A**)&target.store = new A;
+                {
+                    static if (is(A a == U[n], U, size_t n))


This can be shortened to is(A == U[n], U, size_t n)) (a is never used).

MetaLang · 2017-08-06T18:48:58Z

std/variant.d

+                    static if (is(A a == U[n], U, size_t n))
+                    {
+                        pragma(msg, "A");
+                        A* p = cast(A*)(new U[n]).ptr;


Correct me if I'm wrong, but if A == U[n] then typeof((new U[n]).ptr) is already A* and thus there's no need for this cast.

MetaLang · 2017-08-06T18:49:58Z

std/variant.d

                {
                    auto p = new T(rhs);
                }
+                else static if (is(T t == U[n], U, size_t n))


See my above comment for line 343.

MetaLang · 2017-08-06T18:53:36Z

std/variant.d

                }
+                else static if (is(T t == U[n], U, size_t n))
+                {
+                    auto p = cast(T*)(new U[n]).ptr;


See my above comment for line 346.

andralex

thx

andralex · 2017-08-07T20:53:08Z

std/variant.d

+    {
+        int[100] data;
+    }
+    LargeStruct l;


Could you please choose a different name? l looks like 1. Thanks!

Biotronic · 2017-08-08T09:06:20Z

No problem. I'm sorry for not testing it before I pushed the changes - I'm on vacation and my laptop is resistant to getting phobos to compile. :p

…rays > 32(/16) bytes

Biotronic · 2018-01-30T12:57:16Z

Updated. The auto-tester failure looks like a fluke. CircleCI and Jenkins are red on basically every PR. Since the DMD PR is closed, could we get this thing going again?

MetaLang · 2018-01-30T14:44:36Z

I'm not sure what's going on with the testers, but once they're sorted I'll merge this.

wilzbach · 2018-01-30T14:52:19Z

I'm not sure what's going on with the testers

Jenkins: still having a lot of random failures :/
auto-tester: still running into out of memory issues:

make[1]: *** [generated/linux/release/32/unittest/std/regex/internal/tests.o] Killed
make[1]: *** Waiting for unfinished jobs....

CircleCi: known, but I still haven't had time to dig into this.

I restarted Jenkins + deleted the auto-tester job.

but once they're sorted I'll merge this.

Hehe, no please don't wait. Just add auto-merge, it will have a better visibility then.
I typically check the overview here: https://auto-tester.puremagic.com/pulls.ghtml?projectid=1
BTW dlang-bot won't merge automatically if there's a CI failing, but CircleCi + Jenkins aren't enforced, so if the failure looks spurious and everything else is green -> merge ahoy manually :)

wilzbach · 2018-01-30T19:12:13Z

I typically check the overview here: https://auto-tester.puremagic.com/pulls.ghtml?projectid=1

And found this PR and restarted CircleCi as it has been fixed: #6099

Biotronic force-pushed the fix-10879 branch 2 times, most recently from 3258c5b to 6e18dce Compare April 16, 2016 13:46

DmitryOlshansky reviewed Apr 16, 2016
View reviewed changes

Biotronic force-pushed the fix-10879 branch from 6e18dce to 8194ed7 Compare April 16, 2016 22:14

Biotronic mentioned this pull request Apr 20, 2016

Allow creation of static arrays with new dlang/dmd#5688

Closed

quickfur added the Review:Needs Decision label Jul 20, 2016

9il added the Severity:Bug Fix label Jul 29, 2016

dlang-bot added Merge:Needs Rebase Review:Needs Work Merge:stalled labels Jun 14, 2017

dlang-bot removed the Merge:stalled label Jun 22, 2017

Biotronic force-pushed the fix-10879 branch from 8194ed7 to d36f5db Compare July 31, 2017 21:09

Biotronic requested a review from andralex as a code owner July 31, 2017 21:09

Biotronic force-pushed the fix-10879 branch 3 times, most recently from c501deb to 72212e3 Compare August 1, 2017 03:11

MetaLang removed the Review:Needs Work label Aug 6, 2017

MetaLang suggested changes Aug 6, 2017

View reviewed changes

Biotronic force-pushed the fix-10879 branch from 72212e3 to f03a4b3 Compare August 7, 2017 10:36

andralex approved these changes Aug 7, 2017

View reviewed changes

std/variant.d Outdated

{

int[100] data;

}

LargeStruct l;

Copy link

Member

andralex Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please choose a different name? l looks like 1. Thanks!

MetaLang approved these changes Aug 7, 2017

View reviewed changes

MetaLang added Merge:auto-merge and removed Merge:Needs Rebase labels Aug 7, 2017

dlang-bot added Review:Needs Work Merge:stalled Merge:Needs Rebase and removed Merge:Needs Rebase labels Dec 29, 2017

Biotronic force-pushed the fix-10879 branch from e814f07 to 2206d95 Compare January 5, 2018 21:20

dlang-bot removed Review:Needs Work Merge:stalled labels Jan 5, 2018

Biotronic force-pushed the fix-10879 branch 2 times, most recently from 0ed0cf2 to f32df71 Compare January 7, 2018 02:42

dlang-bot added the Review:Needs Work label Jan 7, 2018

Biotronic force-pushed the fix-10879 branch from f32df71 to ba755bf Compare January 7, 2018 16:27

dlang-bot removed the Review:Needs Work label Jan 7, 2018

Biotronic force-pushed the fix-10879 branch from ba755bf to e814f07 Compare January 13, 2018 00:41

dlang-bot added the Review:Needs Work label Jan 13, 2018

Fix Issue 10879 - std.variant Variant/Algebraic: Cant store static ar…

875015e

…rays > 32(/16) bytes

Biotronic force-pushed the fix-10879 branch from e814f07 to 875015e Compare January 28, 2018 12:40

dlang-bot removed the Review:Needs Work label Jan 28, 2018

MetaLang approved these changes Jan 30, 2018

View reviewed changes

wilzbach added the Merge:auto-merge label Jan 30, 2018

wilzbach removed the Review:Needs Decision label Jan 30, 2018

wilzbach merged commit 1fd20cf into dlang:master Jan 31, 2018

Uh oh!

Comments

Conversation

Biotronic commented Apr 16, 2016

Uh oh!

dlang-bot commented Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Biotronic commented Apr 17, 2016

Uh oh!

DmitryOlshansky commented Apr 20, 2016

Uh oh!

Biotronic commented Apr 20, 2016

Uh oh!

wilzbach commented May 10, 2016

Uh oh!

tramker commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetaLang commented Jul 21, 2017

Uh oh!

MetaLang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Biotronic commented Aug 8, 2017

Uh oh!

Biotronic commented Jan 30, 2018

Uh oh!

MetaLang commented Jan 30, 2018

Uh oh!

wilzbach commented Jan 30, 2018

Uh oh!

wilzbach commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dlang-bot commented Apr 16, 2016 •

edited

Loading

tramker commented Nov 6, 2016 •

edited

Loading

MetaLang left a comment •

edited

Loading