Skip to content

Conversation

@darsnack
Copy link
Contributor

@darsnack darsnack commented Jul 28, 2021

DataType no longer has a mutable field. The new function ismutabletype can be used instead. Similarly, abstract is now isabstracttype.

jl_new_datatype now takes in a fattrs argument which I think should be T.atomicfields (maybe @vtjnash can correct me). The change was added in JuliaLang/julia#37847

Only remaining error is that accessing the field N for Vararg{T} is now an undefined reference.

Fixes #107

@darsnack
Copy link
Contributor Author

Okay all the errors should have been resolved @DhairyaLGandhi

@darsnack
Copy link
Contributor Author

It seems we were only running into that issue for Vararg{T}, so I added a specific path for just that case and reverted the isdefined change. I also added some more tests for undefined references. There is one case that is currently broken (i.e. #43).

@vtjnash can you comment on what the fattrs argument to jl_new_datatype is supposed to represent? Seems like field attributes (e.g. atomic), but I wasn't sure what I should be passing in.

@vtjnash
Copy link

vtjnash commented Jul 29, 2021

It is unlikely that you should be trying to access internal functions via ccall. The allocator for struct objects is an intrinsic function: Core._structtype(Main, :A, %3, %4, %5, false, 0) (see @Meta.lower struct A; end), though the other functions called there are also mandatory, so calling eval with a struct definition is perhaps the most reliable version.

@darsnack darsnack force-pushed the darsnack/mutable-fix branch from 168d069 to bf7228b Compare February 12, 2022 15:38
@darsnack
Copy link
Contributor Author

@CarloLucibello I don't have commit rights here, can you review?

The fixes here touch internals, but it is no more hacky than the rest of the package in that regard. A full overhaul to make the package more stable in the long term is needed (we already see failures on nightly), but not a task that I can complete in the near future. I would suggest this is merged so that BSON.jl is usable on 1.7.

@CarloLucibello
Copy link
Collaborator

old tests are passing, new tests are added, good enough for me

@CarloLucibello CarloLucibello merged commit 379fb3c into JuliaIO:master Feb 12, 2022
@darsnack darsnack deleted the darsnack/mutable-fix branch February 12, 2022 15:56
@darsnack
Copy link
Contributor Author

We should tag a patch release too

@CarloLucibello
Copy link
Collaborator

done

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.

Problem saving function in BSON

4 participants