dlang-community / std_data_json

Phobos candidate JSON implementation.
25 stars 13 forks source link

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

Open FeepingCreature opened 4 years ago

FeepingCreature commented 4 years ago

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 https://github.com/dlang-community/std_data_json/pull/51 , this time with a backported version of std_data_json that has actually been tested and actually avoids the linker error from https://github.com/dlang-community/std_data_json/issues/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.

PetarKirov commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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