Open
Conversation
variant.get!Variant used to segfault, but after PR dlang#10697 it only threw an exception. The reason for that was that get!Variant called the `handler` with OpID.get. Handler is a function pointer to a templated function parametrized on the type currently wrapped by Variant. Because Variant generally get flattened, so `Variant(Variant(4))` just wraps the value `4`, instead of `Variant(4)`, the wrapped type (should generally) not be "Variant", and thus the OpID.get operation will fail. This couldn't be fixed in the handler, because OpID.get uses typeinfo for the target type, but VariantN is a template, so afaik we can't really test it against the infinite set of valid VariantN instantiations. `get` still gets the target type as a template parameter, so we can handle the problem there. Supporting get!Variant incidentally also fixed dlang#10518. Adding variant inside an associative array of variants failed: ``` Variant variant = Variant([ "one": Variant(1), ]); variant["four"] = Variant(4); ``` This was because `opIndexAssign` wrapps the index as well as the value you want to assign in a Variant again. ``` Variant opIndexAssign(T, N)(T value, N i) { Variant[2] args = [ Variant(value), Variant(i) ]; fptr(OpID.indexAssign, &store, &args) == 0 || assert(false); return args[0]; } ``` The handler of `variant` containing the AA is parametrized with `Variant[string]`, but opIndexAssign passes a variant of type int instead of Variant(Variant(int)) due to the aformentioned flattening. Eventually in the handler, it ends up calling `Variant(i).get!Variant`, which crashed. For the github automation: Fixes dlang#10431 Fixes dlang#10518
Contributor
|
Thanks for your pull request and interest in making D better, @Inkrementator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
This bug also depended on get!Variant working. Fixes dlang#9980
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
variant.get!Variant used to segfault, but after PR #10697 it only threw an exception.
The reason for that was that get!Variant called the
handlerwith OpID.get. Handler is a function pointer to a templated function parametrized on the type currently wrapped by Variant. Because Variant generally get flattened, soVariant(Variant(4))just wraps the value4, instead ofVariant(4), the wrapped type (should generally) not be "Variant", and thus the OpID.get operation will fail.This couldn't be fixed in the handler, because OpID.get uses typeinfo for the target type, but VariantN is a template, so afaik we can't really test it against the infinite set of valid VariantN instantiations.
getstill gets the target type as a template parameter, so we can handle the problem there.Supporting get!Variant incidentally also fixed #10518. Adding variant inside an associative array of variants failed:
This was because
opIndexAssignwrapps the index as well as the value you want to assign in a Variant again.The handler of
variantcontaining the AA is parametrized withVariant[string], but opIndexAssign passes a variant of type int instead of Variant(Variant(int)) due to the aformentioned flattening.Eventually in the handler, it ends up calling
Variant(i).get!Variant, which crashed.For the github automation:
Fixes #10431
Fixes #10518
Fixes #9980