replace std.variant with std.sumtype #2550
Conversation
Variant is quite big and runtime heavy, with sumtype we already cover wrong type cases at compile time.
|
@WebFreak001 which versions of gdc have you tried? gdc-10, gdc-11, gdc-12? As far as my (attempted) reduction is concerned, only gdc-12 is affected, to which I have a fix prepped for it, and will backport for the next 12 minor release + 13. |
|
only GDC 12 and above would be supported by DUB because of syntax changes in the language For GDC support see #2548 |
source/dub/commandline.d
Outdated
| (OriginalType!T v) => cast(T)v, | ||
| (_) => assert(false, "value from previous getopt has different type than the current getopt call")) |
There was a problem hiding this comment.
Wouldn't that return incorrect results if the previous getopt call set a value outside of T's bounds ?
There was a problem hiding this comment.
yes that could happen, but I think the previous code would have done the same. (just doing it inside the standard library and not in DUB) - It's just an issue for when the programmer makes a mistake anyway and can't usually be caused by the user doing something.
| enforceSDL(false, format("Expected value of type " ~ T.stringof ~ " for '%s', but got %s.", | ||
| customFieldName.length ? customFieldName : errorInfo.fullName, | ||
| typeof(fallback).stringof), | ||
| errorInfo, file, line); |
There was a problem hiding this comment.
We don't pass file/line info for the actual parsed file anywhere ?
|
Failure is due to the unnecessary move BTW. If we were to proceed with the move, you'd have to update the txt file to change the path of |
|
Hey @WebFreak001 @Geod24 what's the status of this PR? |
|
adjusted to review, ready for another round |
| string platform; | ||
| if ("platform" in t.attributes) | ||
| platform = t.attributes["platform"][0].value.get!string; | ||
| dst[platform] ~= t.values.map!(v => v.get!string).array; | ||
| platform = t.attributes["platform"][0].value.expect!string(t, t.fullName ~ " platform", file, line); | ||
| return platform; |
There was a problem hiding this comment.
In the future I'd recommend writing this as:
if (auto ptr = "platform" in t.attributes)
return (*ptr)[0].value...
return null;To make it more obvious what is happening, but you are just matching the old style here.
Variant is quite big and runtime heavy, with sumtype we already cover wrong type cases at compile time.
I was thinking this could be useful in fixing the GDC linker error that's caused by Variant.toString failing with to!string(float) code emission, however while I could fix it with a simple toString replacement for writeln, I couldn't fix it in all the different cases where floats are converted to strings here. Finding the linker error causes is quite difficult and fixing them is very ugly, so I didn't attempt to fix GDC compatibility here. (but using sumtype with explicit types rather than the all-encompassing Variant can certainly help in fixing it)