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

Fix more C warnings #6

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Fix more C warnings #6

merged 2 commits into from
Oct 15, 2024

Conversation

klmr
Copy link
Contributor

@klmr klmr commented Oct 15, 2024

There were some other warnings that were missed in the previous MR. This one is more exhaustive, and builds cleanly for me locally (with many warnings enabled). However, it changes the public signature of the exported AVL tree functions, and as a consequence it might break downstream dependencies. I had a look through the other ribios* packages but I couldn’t find any that used the AVL tree implementation. Still, we might want to exercise caution before merging this change.

There were some other warnings that were missed in the previous MR. This
one is more exhaustive, and builds cleanly for me locally. However, it
changes the public signature of the exported AVL tree functions, and as
a consequence it might break downstream dependencies. I had a look
through the other ribios* packages but I couldn’t find any that used the
AVL tree implemeentation. Still, we might want to exercise caution
before merging this change.
Apparently `%T` is still not supported by all standard library versions
(in particular MinGW), despite being a POSIX standard. We can fix this
easily by instead using `%H:%M:%S`.
@Accio
Copy link
Collaborator

Accio commented Oct 15, 2024

Thanks a lot, daer @klmr. I confirm that the AVL tree implementation is not actively used in other ribios packages. The data structure was introduced by Clemens Broger in the BIOS package for the hash_table, which has not been exposed to R by ribios packages.

I reviewed the changes and they look fantastic. Thank you very much!

@Accio Accio marked this pull request as ready for review October 15, 2024 13:21
@Accio Accio merged commit 0b55540 into master Oct 15, 2024
3 of 4 checks passed
@klmr klmr deleted the fix-more-c branch October 16, 2024 16:15
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