Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix and test datatype field reflection #14777

Merged
merged 1 commit into from
Jan 25, 2016
Merged

fix and test datatype field reflection #14777

merged 1 commit into from
Jan 25, 2016

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 24, 2016

replaces field_offset with corrected, tested, an documented method fieldoffset

this function was untested, undocumented, unexported, and incorrect (off-by-one error in implementation)

see 30a44c9#commitcomment-15624022

@JeffBezanson
Copy link
Member

True, but there is much to be said for this API over fieldoffsets, which allocates an array on every call. There is one use in sparse/cholmod.jl:

1042:        offset = fieldoffsets(C_Sparse)[offidx]

ouch. I think we should deprecate fieldoffsets as well or instead.

@vtjnash
Copy link
Member Author

vtjnash commented Jan 24, 2016

ouch is right. but it's trivial to lift the rest of that computation into the same let block that is computing offidx

@vtjnash vtjnash closed this Jan 24, 2016
@vtjnash vtjnash reopened this Jan 24, 2016
@JeffBezanson
Copy link
Member

Still, fieldoffset would be a better API, and more consistent with fieldtype.

@vtjnash
Copy link
Member Author

vtjnash commented Jan 24, 2016

agreed. i'll switch the PR.

@vtjnash vtjnash changed the title deprecate field_offset fix and test datatype field reflection Jan 24, 2016
@vtjnash vtjnash force-pushed the jn/field-offset branch 2 times, most recently from b37c955 to 64b396b Compare January 24, 2016 06:50
@vtjnash
Copy link
Member Author

vtjnash commented Jan 24, 2016

updated per discussion


@deprecate field_offset(x::DataType, idx) fieldoffset(x, idx+1)
@noinline function fieldoffsets(x::DataType)
depwarn("fieldoffsets deprecated. use `map(idx->fieldoffset(x, idx), 1:nfields(x))` instead", :fieldoffsets)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is deprecated

`field_offset` was untested, undocumented, unexported, and incorrect (off-by-one error in implementation)
`fieldoffset` just isn't particularly optimal, since it has to allocate a new array each time
also make minor cleanup and test-coverage enhancements to related reflection methods
vtjnash added a commit that referenced this pull request Jan 25, 2016
fix and test datatype field reflection
@vtjnash vtjnash merged commit 85f2659 into master Jan 25, 2016
@vtjnash vtjnash deleted the jn/field-offset branch January 25, 2016 20:16
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.

3 participants