-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Feature/issue383 -- closes #383 #397
Conversation
Bump to @jjallaire @kevinushey -- please take a look when time permits. This is an addition of three free functions (along with tests), and changes no interface. |
LGTM however I don't know enough about the internals to fully provide the green light. @kevinushey ? |
s[i] = x[j]; | ||
} | ||
|
||
// there must be a simpler, more C++-ish way for this ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do something like this:
SEXP oldDimNames = Rf_getAttrib(x, R_DimNamesSymbol);
if (!Rf_isNull(oldDimNames)) {
Shield<SEXP> newDimNames(Rf_allocVector(VECSXP, 2));
SET_VECTOR_ELT(newDimNames, 0, VECTOR_ELT(oldDimNames, 1));
SET_VECTOR_ELT(newDimNames, 1, VECTOR_ELT(oldDimNames, 0));
Rf_setAttrib(r, R_DimNamesSymbol, newDimNames);
}
Marginally cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Anything that avoid PROTECT
and UNPROTECT
has my vote :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's been committed.
This is fine as-is but made some comments that might help clean up (code duplication, |
Very helpful -- thanks for he comments, Kevin. Will revise once more. |
Bump. @kevinushey if you can take another quick look... |
Looks good to me! |
Feature/issue383 -- closes #383
this should close #383:
dimnames