improve compatibility for c++ #1191
Conversation
| size_t hts_realloc_or_die(size_t, size_t, size_t, size_t, | ||
| int, void **, const char *); |
There was a problem hiding this comment.
Havind to declare this is mildly annoying, as we don't want it to be called directly. I guess not listing what any of the parameters are makes this less likely, but it would be good to at least have a comment above it saying "Do not call this function directly, only use it via the hts_expand() and hts_expand0() macros".
We could alternately define a few extra macros so we can wrap extern "C" { ... } around the existing declarations in hts_expand() and hts_expand0(). It does end up being a bit ugly though, so maybe just having the "do not use" comment would be the best solution.
There was a problem hiding this comment.
It turns out you can't put extern "C" on the existing block-local declarations as it's only valid at namespace scope.
But why is C++ code using hts_expand() or hts_expand0() anyway? Both macros are documented as “Do not use this macro”, and C++ code in particular has better facilities available to it.
There was a problem hiding this comment.
This did cross my mind.
@s-wakaba What C++ code is using these macros?
There was a problem hiding this comment.
I'm making my bam manipulation tool based on code of samtools and these macros are used in it. I hadn't noticed that these macros are obsoleted. Is it better to update samtools code?
There was a problem hiding this comment.
The replacement for hts_expand() and hts_expand0() is hts_resize(). The difference is that instead of killing your program if it runs out of memory, hts_resize() returns an error code which means you can handle the problem more gracefully. You do have to check the return value and handle the error condition yourself, though.
hts_expand() is fine in cases where you don't mind the program exiting if you've run out of memory. For example samtools will generally give up anyway if you run out of memory, so hts_expand() just makes it happen in a faster and possibly less tidy way.
For c++ code, I'd have thought you would use c++ features like vector<> and exceptions to get growable memory allocations. But I guess this depends on how the data needs to interact with HTSlib structures.
I note that the helper function used by hts_resize() needs to be visible in the header file due to the way hts_resize() works, so I guess we can live with hts_realloc_or_die() being the same as long as it gets a similar comment.
| @@ -88,7 +88,11 @@ typedef struct { | |||
| int depth; | |||
| } ks_isort_stack_t; | |||
|
|
|||
| #ifndef __cplusplus | |||
There was a problem hiding this comment.
You could probably just get rid of the register annotation. I doubt that it makes much difference for modern compilers.
Current header files of htslib cannot be included in c++ code without correction.
Function declarations in macro function are put out of "extern C" in c++ code, and they trigger linkage errors.
Implicit cast from void to typed pointer is forbidden.
"register" keyword is not recommended in c++ code.