Skip to content
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

improve compatibility for c++ #1191

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

s-wakaba
Copy link

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.

Comment on lines +62 to +63
size_t hts_realloc_or_die(size_t, size_t, size_t, size_t,
int, void **, const char *);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jmarshall jmarshall Dec 15, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This did cross my mind.

@s-wakaba What C++ code is using these macros?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

You could probably just get rid of the register annotation. I doubt that it makes much difference for modern compilers.

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.

3 participants