Replace ATTRIB, SET_ATTRIB
#7487
Merged
+174
−110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead, backport and use CLEAR_ATTRIB (R >= 4.5).
Use SHALLOW_DUPLICATE_ATTRIB (R >= 3.3) for the simple case. Also, Backport ANY_ATTRIB (R >= 4.5) instead of testing !isNull(ATTRIB(.)).
Instead of trying to walk ATTRIB in search of the compact 'rownames' attribute to modify, install it anew, take note of the returned reference to the value being installed (a different one!) and modify that.
Instead of walking the attribute list directly, use R_mapAttrib(). Create a hash table of index names instead of relying on chin() and a temporary string vector. Move all temporary allocations onto the R heap.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7487 +/- ##
==========================================
+ Coverage 98.99% 99.02% +0.03%
==========================================
Files 87 87
Lines 16729 16750 +21
==========================================
+ Hits 16561 16587 +26
+ Misses 168 163 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Generated via commit 069560c Download link for the artifact containing the test results: ↓ atime-results.zip
|
Member
HughParsonage
left a comment
HughParsonage
left a comment
There was a problem hiding this comment.
Maybe add some brief documentation about what each function does and returns if time permits.
Member
Author
|
This is expected to become more of a problem around end of January. |
ben-schwen
reviewed
Dec 26, 2025
ben-schwen
reviewed
Dec 26, 2025
| R_len_t ngrp, nrowgroups, njval=0, ngrpcols, ansloc=0, maxn, estn=-1, thisansloc, grpn, thislen, igrp; | ||
| int nprotect=0; | ||
| SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, rownames, s, RHS, target, source; | ||
| SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, s, RHS, target, source; |
Member
There was a problem hiding this comment.
Suggested change
| SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, s, RHS, target, source; | |
| SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, RHS, target, source; |
ben-schwen
reviewed
Dec 26, 2025
Co-authored-by: Benjamin Schwendinger <[email protected]>
Otherwise the callback could remove the attribute and end up with the value unprotected. Protect the attribute tag as well for uniformity. Co-Authored-By: HughParsonage <[email protected]>
This solution is closer to the working approach previously taken by the code.
ben-schwen
approved these changes
Jan 8, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Towards #6180.
Replace remaining uses of
SET_ATTRIBwithSHALLOW_DUPLICATE_ATTRIBorCLEAR_ATTRIB. Replace remaining uses ofATTRIBtesting withANY_ATTRIB; use the newR_mapAttribinterface to walk the attributes.assign.cwhereattr(., 'index')is adjusted following a by-reference operation that could invalidate some indices. What used to be afor (cons = ATTRIB(x); !isNull(cons); cons = CDR(cons))loop now has to construct a contextstructto share between the caller and the map callback. On the plus side, I simplified the memory management and string operations a bit, getting rid of things likememset(string, '\0', 1)and making use of the new hash table to look up the indices.rownamesattribute indogroups.cis also a bit tricky. Nowdogroupscreates a throwawayINTSXPc(NA_integer, -whatever), gives it tosetAttrib(SD, R_RowNamesSymbol, decoy), and obtains the actual compactrownamesattribute that R re-creates from the return value. I think this is simpler than walking the attribute list by hand in search of the correct attribute, and the end result is the same: we can modifyrownamesby reference without spilling it into a large string vector.