Skip to content

Small simplification removing extra layer (closes #8) #9

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

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

eddelbuettel
Copy link
Owner

This PR removes the try / catch block from four alignment helpers. Rcpp automatically adds such a block by expanding the BEGIN_RCPP and END_RCPP macros inserted by compileAttributes(), and also uses an approach to report the error back that has seen some refinement. This should help with the small valgrind leak reported at CRAN -- and does so in local testing.

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ea49578) 100.00% compared to head (f924615) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master        #9   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          307       304    -3     
=========================================
- Hits           307       304    -3     
Impacted Files Coverage Δ
src/align.cpp 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@eddelbuettel eddelbuettel marked this pull request as draft August 21, 2022 12:46
@eddelbuettel
Copy link
Owner Author

Actually, updating to data.table 0.14.3 as it is right now at its repo as opposed to the variant I had changes things again so I market this as draft, and will update the issue #8.

@eddelbuettel
Copy link
Owner Author

Yikes. I had forgotten about this. Still an issue we need to get to.

I can reproduce with data.table from CRAN as well as the dev version. A simple R CMD check --use-valgrind is enough; it does not stop the run but file dtts.Rcheck/tests/tinytest.Rout has something.

@eddelbuettel eddelbuettel force-pushed the feature/small_refactor branch from 5b60e83 to f924615 Compare March 22, 2023 23:33
@eddelbuettel eddelbuettel marked this pull request as ready for review March 23, 2023 00:13
@eddelbuettel eddelbuettel requested a review from lsilvest March 23, 2023 00:15
@eddelbuettel
Copy link
Owner Author

Ok, undrafted the PR. We both know it does not improve the valgrind issue by itself, but it helps with a marginal small improvement.

@lsilvest lsilvest merged commit a5fbcfe into master Mar 23, 2023
@lsilvest lsilvest deleted the feature/small_refactor branch March 23, 2023 02:39
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.

2 participants