Skip to content

Conversation

ahl27
Copy link
Contributor

@ahl27 ahl27 commented Dec 20, 2024

Adds analogous fwrite method for oZFile accessors. fwrite tends to be slightly faster than puts since it doesn't have to check every character for '\0', which allows for some OS optimization. Would be useful to use in Biostrings for writeXStringSet since the write size of blocks are known in advance, so appending '\0' and then asking the OS to check for it is overkill.

Function arguments follow conventions of the other oZFile* functions despite differing from fwrite argument syntax.

gzfwrite is defined here:

https://github.com/madler/zlib/blob/ef24c4c7502169f016dcd2a26923dbaf3216748c/gzwrite.c#L260-L262

Header definition is here:

https://github.com/madler/zlib/blob/ef24c4c7502169f016dcd2a26923dbaf3216748c/zlib.h#L1472-L1473

I moved the n==nitems check to the end of the switch case because I find it significantly more readable than the alternative in oZFile_puts.

@ahl27 ahl27 changed the title Add oZFile_fwrite method Add oZFile_fwrite method Dec 20, 2024
@ahl27
Copy link
Contributor Author

ahl27 commented Dec 20, 2024

force pushes are some minor commit amends

@ahl27
Copy link
Contributor Author

ahl27 commented Jan 6, 2025

I'm realizing that I forgot to add this method into the exported stubs -- I'm figuring out how to do that, will update this week.

stubs etc. are now added

@hpages
Copy link
Contributor

hpages commented Jan 7, 2025

Looks good. Glad you figured out the DEFINE_CCALLABLE_STUB thing. There's sooo much bureaucracy involved in making a C function available to other packages! I wish it was more straightforward 😞

Speaking of which, there's one more step to the process: you need to register filexp_fwrite with REGISTER_CCALLABLE(filexp_fwrite);. See R_init_XVector.c. As I said, way too much bureaucracy involved!

Only one little nitpick, hope you don't mind: use consistent order for the new stuff e.g. oZFile_fwrite should go after oZFile_putc in io_utils.c because that's the choice you've made for _filexp_fwrite (it comes after _filexp_putc). This is also the choice you've made in XVector.h, XVector_interface.h, and _XVector_stubs.c.

FWIW, don't fill feel the obligation to stick to the old "declare all variables at the beginning of a function" paradygm. The C code in XVector is old, and, at the time I wrote it, I was just into the habit of doing that (this was the only way when I started to code in C in the 90's). But not anymore! I dropped this old habit in my more recent packages like the SparseArray package. Declaring variables on the spot where they are actually used (or about to be used) is perfectly fine if you want to do that. Makes the code a little bit clearer, shorter, easier to maintain, etc...

FWIW, there are other things that I do slightly differently these days e.g. I'm no longer putting all C function declarations in a big monolitic file like XVector.h or Biostrings.h. Instead, I use a collection of header files, one per .c file. Also all my .Call entry points are now prefixed with C_. At some point, I'd like to do the same in XVector and Biostrings but this is really low priority.

Thanks!

@ahl27
Copy link
Contributor Author

ahl27 commented Jan 7, 2025

you need to register filexp_fwrite with REGISTER_CCALLABLE(filexp_fwrite);

Whoops, I totally forgot about that 😅 thanks for the catch! I remembered to do that in the other PRs I have open, but I guess it slipped through the cracks here.

use consistent order for the new stuff

Totally makes sense, I appreciate the feedback. I've just made that change to the code.

FWIW, don't fill the obligation to stick to the old "declare all variables at the beginning of a function" paradygm.

Makes sense haha...I'm a big fan of the Fortran-style syntax with declarations before code, especially in smaller functions, but I'll keep that in mind for the future.

FWIW, there are other things that I do slightly differently these days...

Makes sense, I do something similar in SynExtend. That would probably be a nice change for code organization and future maintainability, but like you mentioned it's not super high priority and would entail a good bit of work.

Thanks for the feedback! Just pushed changes in response to your comments.

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