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

Fixed bug where setattr crashed on character(0) value. #2628

Merged
merged 5 commits into from
Feb 18, 2018
Merged

Conversation

MarkusBonsch
Copy link
Contributor

Closes #2386 .
setattr(1, "class", character(0)) used to cause a segmentation violation. Now, it throws a proper error.

@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #2628 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2628      +/-   ##
==========================================
+ Coverage   93.03%   93.03%   +<.01%     
==========================================
  Files          61       61              
  Lines       12115    12116       +1     
==========================================
+ Hits        11271    11272       +1     
  Misses        844      844
Impacted Files Coverage Δ
R/data.table.R 97.61% <100%> (ø) ⬆️
src/wrappers.c 95.08% <100%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01ef8e3...f2b0497. Read the comment docs.

@mattdowle
Copy link
Member

mattdowle commented Feb 15, 2018

Great find. I guess this will fail on any object, not just 1; that 1 was just to illustrate? If so, the side-issue then with 1L and possibly 1 too is that they could be R's internal length-1 globals. Then setattr() could potentially add an attribute to the global value of 1L. Similarly to how data.table could change the global value of TRUE to FALSE several years ago in R-devel before release that was caught early on via tests against R-devel . Will use this PR as a nudge to look into length-1 globals status in R-devel/altrep.

@MarkusBonsch
Copy link
Contributor Author

@mattdowle Wow, I didn't even know these things existed :). You are completely right, 1 was just to illustrate. I replaced with data.table(x=1:10) in the tests in order to avoid side-effects.

@mattdowle mattdowle added this to the v1.10.6 milestone Feb 18, 2018
@mattdowle mattdowle merged commit aa50640 into master Feb 18, 2018
@mattdowle mattdowle deleted the setattrBug branch February 18, 2018 01:00
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