Closed frebib closed 3 years ago
All comments addressed, I think. Main changes are:
.
and capital letters were fixedInto()
will prefer to return an error
instead of panic()
in every case. If it panic()
s then it'll be from the reflect
library, and is likely indicative of a bug in the Into()
implementation.🙈 Had to fix-up some go vet
complaints about incorrectly reflowed comments, but I think we're good now. Running a full make test
on this just to be sure
I've fixed up the err :=
instances. It looks like I forgot to stage/commit the additional return
statements; they were still in my working tree. Hopefully I didn't miss anything
19:09 < purpleidea> frebib: Your PR failed because the first word in the commit message AFTER the prefixes needs to be capitalized... 19:09 < purpleidea> eg: lang: Move StructTag const etc 19:09 < purpleidea> OR 19:09 < purpleidea> example: lang: Reinstate mcl blah
Can you capitalize the first word after the prefixes and resubmit please? Sorry about that.
Fixed up the capitalisation in the commit messages
A very fantastic patchset merged. Thank you again!
Into() mutates a given reflect.Value and sets the data represented by the types.Value into the storage represented by the reflect.Value.
Into() is the opposite to ValueOf() which converts reflect.Value into types.Value, and in theory they should (almost) bijective with some edge case exceptions where the conversion is lossy.
Simply, it replaces reflect.Value.Set() in the broad case, giving finer control of how the reflect.Value is modified and how the data is set. types.Value.Value() is now also a redundant function that achieves the same outcome as Into(), but with less type specificity.
lang: convert StmtRes to engine.Res with types.Into()
Replace existing field-mapping code with calls to types.Into() to reflect the mcl data into the Go resource struct with finer granularity and accuracy, and less reliance on the magic reflect.Set() function.o
One major advantage over reflect.Value.Set() is Into() allows tailoring the data that is set, specifically when coercing mcl struct values into Golang struct values, the fields can be appropriately mapped based on the lang tag on the struct field. With reflect.Value.Set() this was not at all possible as there was a contradiction of logic given the following rules:
Prior to this change, it was impossible to map an mcl inline struct in a resource to the matched Golang counterpart, even if the lang tag was present on the struct field. This can be demonstrated with the following trivial example:
and the accompanying Golang resource definition:
Due to the mismatch in field names in the embedded struct, the type unifier failed and refused to match mcl 'name' to Go 'Name' due to the missing mapping logic.
lang: map Go struct fields using
lang
struct tagConverting a reflect.Type of KindStruct did not respect the
lang
tag on struct fields incidating how fields from mcl structs should be mapped even though resource field names did. This patch should allow structs with mapped fields to be respected.Signed-off-by: Joe Groocock me@frebib.net