Skip to content
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

Issue 2950 - Allow switch on enum of string base type. #1358

Closed
wants to merge 1 commit into from
Closed

Issue 2950 - Allow switch on enum of string base type. #1358

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 8, 2012

http://d.puremagic.com/issues/show_bug.cgi?id=2950

This is only a partial fix since this won't compile with the -g switch. When EnumDeclaration::toDebug is called, it calls cv4_Denum which expects the enum to have an integer base type. It tries to call toInteger method on the enum declaration.

cv4_Denum is a very complicated function, I don't know how to implement a fix there.

@@ -0,0 +1,19 @@
enum E : string { a = "first", b = "second", c = "third" }
Copy link
Member

Choose a reason for hiding this comment

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

The trouble with adding a separate file for each test is it tremendously increases the time necessary to run the test suite. Please instead add it to one of the other test files, which adds very little to the time.

Copy link
Member

Choose a reason for hiding this comment

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

Having too few test files can also be problematic, though, as it decreases the granularity of the status reports if the test suite does not pass. For DMD, this might not be as big a concern, as the test suite is developed in synchrony with it anyway. If you are working on an alternative implementation/port, though, it can be quite annoying if a big test case fails, and you can't quickly judge the scope of the breakage. I'm not sure what the best compromise here is.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, the time it takes the full test suite to run is a significant bottleneck.

I've been doing this style for a long time, and having a few files filled with large numbers of tests works out pretty good.

@ghost
Copy link
Author

ghost commented Dec 10, 2012

Moved the test-case into testenum.d.

@ghost
Copy link
Author

ghost commented Apr 7, 2013

Rebased, waiting for review and green.

@ghost
Copy link
Author

ghost commented Jun 10, 2013

Superseded by #2080.

@ghost ghost closed this Jun 10, 2013
@andralex
Copy link
Member

Apologies for the duplicated work!

@ghost
Copy link
Author

ghost commented Jun 10, 2013

Not necessary, his pull was better :) (and the -g switch works with his pull, although I'm not sure whether it's related to the pull).

This pull request was closed.
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.

4 participants