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

Copypaste-inline taggedalgebraic, try 2. Fixes #41? #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

FeepingCreature
Copy link

@FeepingCreature FeepingCreature commented Sep 24, 2020

Backport of the taggedalgebraic hacks I've done in serialized to sidestep DMD bug https://issues.dlang.org/show_bug.cgi?id=21235 .

This is an update of #51 , this time with a backported version of std_data_json that has actually been tested and actually avoids the linker error from #41 .

Note that these changes can never be upstreamed to taggedalgebraic, because they break opEquals so that it only barely even works for our JSONValue.

This should be safe to revert once the DMD bug is fixed, but I wouldn't count on this happening soon: from what I've seen of it, it's an ugly one, and happens rarely to boot.

@FeepingCreature FeepingCreature changed the title Copypaste-inline taggedalgebraic, try 2 Copypaste-inline taggedalgebraic, try 2. Fixes #41? Sep 24, 2020
@PetarKirov
Copy link
Member

PetarKirov commented Sep 25, 2020

Commits f0749c8 and b85fa6b look good to me (though please update the commit message of f0749c8 to specify the exact git hash of taggedalgebraic you're inlining) but 43496db worries me as it deletes many unit tests. Do all of them need to disabled? Can you at least mark them with @disable or version (none) (along with a TODO comment to enable them when the bug is fixed) to make sure they're not lost, instead of deleting them?

@FeepingCreature
Copy link
Author

FeepingCreature commented Sep 25, 2020

That's deliberate. taggedalgebraic has a lot more functionality than std.data.json needs, and those unittests check for it. I just chucked everything out that triggered errors and didn't seem to be the sort of type that JSONValue involves. Again, this is just a temporary fix until either DMD (please) or taggedalgebraic gets it properly solved, though it's of lesser importance to me since I've just inlined std_data_json into serialized anyways, so I'm trying to upstream my modified version, but it's not urgent.

@Panke
Copy link

Panke commented Dec 11, 2020

I've just run into the bug. Can we go forward with this?

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