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.

Sign up for GitHub

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

Jump to bottom

improve compatibility for c++ #1191

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

improve compatibility for c++ #1191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions htslib/hts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ struct hFILE;
struct hts_tpool;
struct sam_hdr_t;

size_t hts_realloc_or_die(size_t, size_t, size_t, size_t,
int, void **, const char *);
Comment on lines +62 to +63
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 •
edited
Loading

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.

/**
* @hideinitializer
* Deprecated macro to expand a dynamic array of a given type
Expand All @@ -85,8 +87,6 @@ struct sam_hdr_t;
*/
#define hts_expand(type_t, n, m, ptr) do { \
if ((n) > (m)) { \
size_t hts_realloc_or_die(size_t, size_t, size_t, size_t, \
int, void **, const char *); \
(m) = hts_realloc_or_die((n) >= 1 ? (n) : 1, (m), sizeof(m), \
sizeof(type_t), 0, \
(void **)&(ptr), __func__); \
Expand Down Expand Up @@ -117,8 +117,6 @@ struct sam_hdr_t;

#define hts_expand0(type_t, n, m, ptr) do { \
if ((n) > (m)) { \
size_t hts_realloc_or_die(size_t, size_t, size_t, size_t, \
int, void **, const char *); \
(m) = hts_realloc_or_die((n) >= 1 ? (n) : 1, (m), sizeof(m), \
sizeof(type_t), 1, \
(void **)&(ptr), __func__); \
Expand Down
8 changes: 4 additions & 4 deletions htslib/klist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
kmptype_t **buf; \
} kmp_##name##_t; \
SCOPE kmp_##name##_t *kmp_init_##name(void) { \
return calloc(1, sizeof(kmp_##name##_t)); \
return (kmp_##name##_t *)calloc(1, sizeof(kmp_##name##_t)); \
} \
SCOPE void kmp_destroy_##name(kmp_##name##_t *mp) { \
size_t k; \
Expand All @@ -54,14 +54,14 @@
} \
SCOPE kmptype_t *kmp_alloc_##name(kmp_##name##_t *mp) { \
++mp->cnt; \
if (mp->n == 0) return calloc(1, sizeof(kmptype_t)); \
if (mp->n == 0) return (kmptype_t *)calloc(1, sizeof(kmptype_t)); \
return mp->buf[--mp->n]; \
} \
SCOPE void kmp_free_##name(kmp_##name##_t *mp, kmptype_t *p) { \
--mp->cnt; \
if (mp->n == mp->max) { \
mp->max = mp->max? mp->max<<1 : 16; \
mp->buf = realloc(mp->buf, sizeof(kmptype_t *) * mp->max); \
mp->buf = (kmptype_t **)realloc(mp->buf, sizeof(kmptype_t *) * mp->max); \
} \
mp->buf[mp->n++] = p; \
}
Expand All @@ -88,7 +88,7 @@
size_t size; \
} kl_##name##_t; \
SCOPE kl_##name##_t *kl_init_##name(void) { \
kl_##name##_t *kl = calloc(1, sizeof(kl_##name##_t)); \
kl_##name##_t *kl = (kl_##name##_t *)calloc(1, sizeof(kl_##name##_t)); \
kl->mp = kmp_init(name); \
kl->head = kl->tail = kmp_alloc(name, kl->mp); \
kl->head->next = 0; \
Expand Down
4 changes: 4 additions & 0 deletions htslib/ksort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

#define KSORT_SWAP(type_t, a, b) { register type_t t=(a); (a)=(b); (b)=t; }
#else
#define KSORT_SWAP(type_t, a, b) { type_t t=(a); (a)=(b); (b)=t; }
#endif

#define KSORT_INIT(name, type_t, __sort_lt) KSORT_INIT_(_ ## name, , type_t, __sort_lt)
#define KSORT_INIT_STATIC(name, type_t, __sort_lt) KSORT_INIT_(_ ## name, static klib_unused, type_t, __sort_lt)
Expand Down