Skip to content

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

Open
FeepingCreature wants to merge 3 commits into
dlang-community:masterfrom
funkwerk:fix/taggedalgebraic
Open

Copypaste-inline taggedalgebraic, try 2. Fixes #41?#52
FeepingCreature wants to merge 3 commits into
dlang-community:masterfrom
funkwerk:fix/taggedalgebraic

Conversation

@FeepingCreature

@FeepingCreature FeepingCreature commented Sep 24, 2020

Copy link
Copy Markdown

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

PetarKirov commented Sep 25, 2020

Copy link
Copy Markdown
Member

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

FeepingCreature commented Sep 25, 2020

Copy link
Copy Markdown
Author

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

Panke commented Dec 11, 2020

Copy link
Copy Markdown

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

@s-ludwig

Copy link
Copy Markdown
Collaborator

Is this still necessary? Can this be fixed in taggedalgebraic directly instead?

@FeepingCreature

Copy link
Copy Markdown
Author

Feel free to close if you don't want it- as mentioned, I'm using an inlined fork of std.data.json anyways.

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