Fix 14561 - Large enums cannot be parsed due to too many recursive template expansions #3275
Conversation
|
Fails on autotester, and indentation needs fixing. |
|
PR title should probably be expanded: http://wiki.dlang.org/Pull_Requests#Create_a_pull_request
|
schveiguy
changed the title
Yes, thanks for noticing. That's a pet peeve of mine. |
|
ping |
|
@Biotronic can you update this pull so that it applies cleanly? |
|
ping @Biotronic |
|
I'm back, baby! Sorry about the wait, I've had the last few months of my life ruined by near-constant migraines, but have finally found medication that works. |
|
Good to hear! Could you also squash the commits? |
|
Is that good enough, or should I go all the way back to f9e0862? |
|
What is 0e3b7ff for? It looks a bit fishy to me. Technically we're supposed to be working on topic branches so that we can merge back to master here as a single branch. But no matter, I don't think it's a big deal. LGTM. |
|
It's when I pulled from https://github.com/D-Programming-Language/phobos/. Maybe I'm doing it wrong? |
|
The idea would be to rebase your work on top of the new master branch, this way avoiding to create a merge commit. |
| @@ -3467,6 +3474,45 @@ unittest | |||
| static assert(__traits(identifier, EnumMembers!E[2]) == "b"); | |||
| } | |||
|
|
|||
| unittest { // Huge enums | |||
| enum TLAs { aa, ab, ac, ad, ae, af, ag, ah, ai, aj, ak, al, am, an, ao, ap, aq, ar, as, at, | |||
There was a problem hiding this comment.
Wouldn't it be acceptable to use template meta programming to generate this ?!
string genEnum()
{
string result = "enum TLAs {";
foreach(c0; 'a'..'z')
foreach(c1; 'a'..'z')
{
result ~= '_';
result ~= c0;
result ~= c1;
result ~= ',';
}
result ~= '}';
return result;
}
mixin(genEnum);Same comment for alias LongList, upper.
Biotronic
force-pushed
the
master
branch
2 times, most recently
from
38309b6 to
b68cf53
Compare
|
Cleaned up commits and unit test code. I seem to be getting the hang of this Git thing now. (Please correct me if I'm wrong :p) |
| @@ -3459,6 +3466,31 @@ unittest // duplicated values | |||
| static assert([ EnumMembers!A ] == [ A.a, A.b, A.c, A.d, A.e ]); | |||
| } | |||
|
|
|||
| unittest // huge enums | |||
There was a problem hiding this comment.
Could you comment with \\ bugzilla 14561 instead ? It seems that this is widely done for unittest realted to bug fixes. If someone read the code, with \\ huge enums, it's not clear that it used to be a bug.
There was a problem hiding this comment.
@bbasile you mean // bugzilla 14561, right?
…mplate expansions
|
LGTM |
|
Yes plz, this is a bug fix. |
|
Auto-merge toggled on |
|
Did this PR just raise the limit? It sounds like it was intended to work with any size enums, but on a large enum of about 20,000 I get recursive template expansion errors still. |
|
A phobos version that includes the fix is not yet released, the latest release was a dot release so it just included the regressions. (pulled the 9 of april in master and 2.071 was released the 5.) So it's ok if it doesn't work, unless you use a nightly or a version build from the master branch, in which case this would be a problem. |
|
@Laeeth if you need this commit, please check out the nightly releases: https://dlang.org/download.html#dmd-nightly |
|
@Laeeth: Yes, it only raises the limit. It changes the implementation to a divide-and-conquer pattern, so the limit is 2^500 instead of 500. You're likely to run into other problems at that point. |
|
Thanks, for the colour everyone. I switched to using a file loaded at launch instead. |
As discussed in https://issues.dlang.org/show_bug.cgi?id=14561, this pull makes EnumMembers use divide-and-conquer (through staticMap), and adds to NoDuplicates support for lists longer than 500 elements.