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

Add C++ casts for external headers. #1683

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

jkbonfield
Copy link
Contributor

Klist.h and kseq.h use calloc, realloc and memchr in static inline code, but if we wish to permit our external headers to be used from a C++ include then these all need explicit casts as "void *" doesn't work the same in C++ as in C.

It's tempting to use extern "C" in an #ifdef __cplusplus guard, but the nature of these pseudo-template klib headers is such that the code will be expanded up later inside a C++ file so the extern "C" doesn't solve it.

See #1674 and #1682

htslib/klist.h Outdated
kl->mp = kmp_init(name); \
kl->head = kl->tail = kmp_alloc(name, kl->mp); \
kl->head = kl->tail = kmp_alloc(name, kl->mp); \
Copy link
Member

Choose a reason for hiding this comment

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

Was touching this line an accident? OTOH the lines with the new casts could benefit from adjusting their backslashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of. I started updating the backslashes to be neat and then just realised the true scale of the horror so backed out of it, mostly... I spotted it after submitting the PR but thought it's largely irrelevant given how messy the rest are! I can undo that line too, or add another separate commit that fixes the hideous whitespace in this file.

It's a bit problematic as it can make comparing upstream trickier, but -w should suffice.

Copy link
Member

Choose a reason for hiding this comment

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

Apart from the newly lengthened lines, the file lines up nicely with 4-space hard tabs. (Which is an abomination in itself, but at least it's upstream's conventional abomination…)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh! It never dawned on me it'd look OK with a non-standard tab width!

Most of the changed lines are now too long, but the one that isn't I've got indenting the same as the others (when using said abomination). Also removed the accidental white space change.

Thanks, even though I now feel slightly ill.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out Heng's down with the kids 😛

Klist.h and kseq.h use calloc, realloc and memchr in static inline
code, but if we wish to permit our external headers to be used from a
C++ include then these all need explicit casts as "void *" doesn't
work the same in C++ as in C.

It's tempting to use `extern "C"` in an `#ifdef __cplusplus` guard,
but the nature of these pseudo-template klib headers is such that the
code will be expanded up later inside a C++ file so the extern "C"
doesn't solve it.

See samtools#1674 and samtools#1682
@daviesrob daviesrob merged commit 4b316cb into samtools:develop Oct 6, 2023
8 checks passed
jmarshall added a commit to jmarshall/pysam that referenced this pull request Apr 23, 2024
Apply the applicable parts of PR samtools/htslib#1683, a followup to
the previous htslib/kseq.h parsing performance improvement that restores
kseq.h's C++ compatibility. Pysam itself uses only C but it turns out
there exist projects with C++ .pyx source that imports pysam.
jmarshall added a commit to jmarshall/pysam that referenced this pull request Apr 23, 2024
Apply the applicable parts of PR samtools/htslib#1683, a followup to
the previous htslib/kseq.h parsing performance improvement that restores
kseq.h's C++ compatibility. Pysam itself uses only C but it turns out
there exist projects with C++ .pyx source that imports pysam.
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