-
Notifications
You must be signed in to change notification settings - Fork 12
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 rchk issues #40
Fix rchk issues #40
Conversation
Fixes for the record* functions, and some work on xx_atobject_entry
@matthewwiese this is great! Thanks for taking a deeper look into this. I'll try to give more detailed feedback on Wednesday or Friday of this week. Again, thank you! |
No rush! My own bandwidth is limited at the moment due to work + evening classes. I mentioned in #41 my own puzzlement in terms of tooling. I don't want to make any assumptions about how the development for this package is done, but perhaps updating tooling (among other things) could be a parallel effort to fixing the |
We remove the use of UNPROTECT_PTR, among other things, throughout the code. All PROTECTs are purely stack-based
This fixes rchk errors - and its inclusion ought to trigger tests in GitHub, I think?
I've gone through the bison parser and refactored it quite a bit. This fixes all
The A detail that requires more investigation, however, is this error I encounter when running with garbage collection "torture" (
It doesn't occur for me in the base repo - mostly because the EDIT: gctorture on the base repo passes |
@matthewwiese just updated the GitHub action's trigger. I think this should cause the runner to start shortly in the PR. |
Please pull in the new updates to master into your branch with: git fetch --all
git merge origin/master |
@matthewwiese would it be okay if we moved 9e3e606 into a separate PR that deals with upgrading bison? From #41, the package is designed to be inclusive using a local source approach instead of depending on system libraries. (Hence, we have a bit of an outdated library inside.) |
@matthewwiese regarding https://stackoverflow.com/questions/48666925/meaning-of-string-elt-in-rcpp I think for a deeper dive, please check hadley's https://github.com/hadley/r-internals/blob/master/strings.md |
Done!
Sure thing - I've reverted that commit. Thanks for the clarification.
That SE answer was quite helpful, and thanks for linking the source of it too. I overlooked that part in the R Extensions manual. I'll also take a look at Hadley's resource you linked, as well. |
@matthewwiese I switched the PR branch from merging into |
Strange. Are you able to trigger a workflow manually, or is that not registering too? I haven't a clue since I've only used Actions for simple things myself. Related to the for( i=0; i<n; i++){
SET_STRING_ELT( ans , i, STRING_ELT(CAR(o),0) ) ;
SET_STRING_ELT( names, i, STRING_ELT(getAttrib(CAR(o), install("names")),0) ) ;
o = CDR(o ) ;
} Apparently |
@matthewwiese I'm going to close this PR as we've opted to go the way of an internal R parser. Thank you again for taking a close look at it awhile back. |
Fixes for the the
record*
functions,rchk
's previous output:In addition, some fixes for
xx_atobject_entry
:is now only:
The remaining error is shared by all
xx_atobject_*
functions. It's related to this pattern ofPROTECT
ingans
before returning:I'm still studying the code, as well as the Writing R Extensions manual, and so don't immediately have an idea for a fix.