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

Use checked_get() in place of get() where possible #104

Merged
merged 2 commits into from Jul 5, 2016

Conversation

Projects
None yet
2 participants
@jimhester
Copy link
Member

commented Jun 14, 2016

This ensures we will generate an error rather than crashing on NULL
XPtrs (as happens when round tripping saveRDS()/readRDS().

Fixes #101

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2016

@hadley PTAL

@hadley

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

LGTM. But why does get() even exist if there's checked_get()??

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2016

If you want to check for NULL yourself and not error.

if (node.get() == NULL) {
  return CharacterVector()
}

ect.

@jimhester

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2016

Plus I there is slight overhead in the check you can avoid using get()

@hadley

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

Still, I think it would be better to have get() and unsafe_get() or similar.

@hadley

This comment has been minimized.

Copy link
Member

commented Jul 3, 2016

Can you add a bullet to news and merge please?

jimhester added some commits Jun 14, 2016

Use checked_get() in place of get() where possible
This ensures we will generate an error rather than crashing on NULL
XPtrs.

@jimhester jimhester force-pushed the jimhester:bugfix/issue101 branch from d498bc9 to 84ef240 Jul 5, 2016

@jimhester jimhester merged commit a360cdf into r-lib:master Jul 5, 2016

2 of 3 checks passed

codecov/patch 73.33% of diff hit (target 76.53%)
Details
codecov/project 77.17% (+0.64%) compared to 5642717
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.