Skip to content

Allow C-level calls to forder#4017

Closed
mattfidler wants to merge 2 commits intoRdatatable:masterfrom
mattfidler:master
Closed

Allow C-level calls to forder#4017
mattfidler wants to merge 2 commits intoRdatatable:masterfrom
mattfidler:master

Conversation

@mattfidler
Copy link
Copy Markdown

@mattfidler mattfidler commented Oct 31, 2019

Closes #4015

I don't actually think this is a duplicate; It allows calling the underlying C code with evaluating in R, which could be faster.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 31, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4017      +/-   ##
==========================================
+ Coverage   99.41%   99.41%   +<.01%     
==========================================
  Files          72       72              
  Lines       13904    13905       +1     
==========================================
+ Hits        13823    13824       +1     
  Misses         81       81
Impacted Files Coverage Δ
src/init.c 100% <100%> (ø) ⬆️

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 5742d79...d09cd7f. Read the comment docs.

@mattdowle
Copy link
Copy Markdown
Member

mattdowle commented Dec 19, 2019

@lsilvest this PR by @mattfidler exports one more C function. It didn't have the & prefix on the function so I added that to be consistent with your #4084 on the assumption you had tested subsetDT and that's working from your package. Then it seemed cleaner to drop the C prefix in the name since it's already clear from having to call R_GetCCallable that it's a C function that data.table is exporting. So I dropped the C prefix from subsetDT in this PR too. Hope ok.

@mattdowle mattdowle added this to the 1.12.9 milestone Dec 19, 2019
Comment thread src/init.c
R_RegisterCCallable("data.table", "CsubsetDT", (DL_FUNC) &subsetDT);

R_RegisterCCallable("data.table", "subsetDT", (DL_FUNC) &subsetDT);
R_RegisterCCallable("data.table", "forder", (DL_FUNC) &forder);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add to ?cdt as well right

@mattdowle
Copy link
Copy Markdown
Member

mattdowle commented Dec 19, 2019

Needs addition to the news item about subsetDT, and addition to ?cdt, please.
Also needs a comment adding to the argument list of forder() reminding us that it's exported and we can't make changes to it in future as easily. Like the comment Jan added to subsetDT in #4084 iirc.
Tagged WIP. When it's untagged WIP I'll look again and merge.

I invited you to be project member, @mattfidler. This enabled you to create branches in the main project in future. You'll need to accept the invitation which shows as a button on your GitHub profile or GitHub repository page. Welcome!
Whether adding one line to export a C functions counts as a code contribution for the purpose of adding to contributor list in DESCRIPTION, I'm not sure.

@mattdowle mattdowle added the WIP label Dec 19, 2019
@mattfidler
Copy link
Copy Markdown
Author

Sorry Missed your comment @mattdowle. There is alot of traffic on data.table.

Thank you for the excellent package. I don't need to be a contributor for this (in my opinion). However CRAN may differ from my opinion (they have in the past). I am fine adding myself as a contributor if I need to.

@mattfidler
Copy link
Copy Markdown
Author

I also couldn't see a project member email or any buttons; Perhaps the invitation has expired.

@jangorecki
Copy link
Copy Markdown
Member

@mattfidler are you planning to incorporate the feedback (cdt manual, news file, description file, comment in forder.c)? or you are not interested anymore in completing this PR?

@mattdowle mattdowle modified the milestones: 1.13.1, 1.13.3 Oct 17, 2020
@mattfidler
Copy link
Copy Markdown
Author

I realized this is still open. I had forgot about this

@mattfidler mattfidler closed this Apr 28, 2022
@jangorecki jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
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.

Export C level forder for use by other packages' C code

4 participants