Skip to content

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented Jan 14, 2014

The array used in the test is bigger than the maximal size which can be stored in a Variant on ARM.
See http://d.puremagic.com/issues/show_bug.cgi?id=10879 for details.

Note: This is part of a set of changes which are required to get test suite & unit tests passing on ARM GDC. All necessary changes for GDC are here for reference:
https://github.com/jpf91/GDC/commits/arm-old

Once these pull requests are merged upstream I'll merge them into GDC as well.

@monarchdodra
Copy link
Collaborator

I don't have much knowledge on how variants work, but seeing you changing this unittest shows there is some portability issues in a library type.

If I was an end user, is there some sort of static check I could do to know if it works? And if so, could we also use that check in the unittest? It would implicitly document how to (potentially) deal with such issues in user code.

@jpf91
Copy link
Contributor Author

jpf91 commented Jan 15, 2014

Variant is defined as

Variant is an alias for VariantN instantiated with the largest of creal, char[], and void delegate()
http://dlang.org/phobos/std_variant.html#.Variant

So the maximum storeable size differs between architectures by definition. The biggest type is usually creal which has a size of 2*real.sizeof. Real is 16 bytes on X86, so you can store 32 bytes on X86. Real is only 8 bytes on ARM and you can only store 16 bytes on ARM.

The problem here is really the unit test cause it tries to store a value bigger than the maximum allowed. (And maybe Variant should heap-allocate in this case, but this is a different issue).

@monarchdodra
Copy link
Collaborator

Variant is an alias for VariantN instantiated with the largest of creal, char[], and void delegate()
http://dlang.org/phobos/std_variant.html#.Variant

So the maximum storeable size differs between architectures by definition.

OK, that works for me.

Real is 16 bytes on X86

10 actually.

@monarchdodra
Copy link
Collaborator

Auto-merge toggled on

@yebblies
Copy link
Contributor

Real is 16 bytes on X86

10 actually.

It's padded to 16 on some OSs.

@monarchdodra
Copy link
Collaborator

It's padded to 16 on some OSs.

Did not know that. Thanks for the info :)

monarchdodra added a commit that referenced this pull request Jan 16, 2014
std.variant: Adjust test for ARM
@monarchdodra monarchdodra merged commit a791f3b into dlang:master Jan 16, 2014
@jpf91
Copy link
Contributor Author

jpf91 commented Jan 16, 2014

thanks

@jpf91 jpf91 deleted the arm-2 branch January 16, 2014 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants