Fix warnings, add CMakeLists.txt, break out bswap64 #28
Conversation
| bytes[5] = (v >> 16) & 0xff; | ||
| bytes[6] = (v >> 8) & 0xff; | ||
| bytes[7] = (v >> 0) & 0xff; | ||
| *(qoa_uint64_t*)bytes = qoa_bswap64(v); |
There was a problem hiding this comment.
You're assuming that the current machine is little endian and you have to byteswap. It will not break on big endian machines. Before the code didn't make any assumptions and worked on everything.
| elseif (UNIX) | ||
| target_link_libraries(QOAConv m pthread asound) | ||
| target_link_libraries(QOAConv -lm) |
There was a problem hiding this comment.
You added this file (and are the reason you need a fix now), why don't you just squash it into the original commit?
There was a problem hiding this comment.
Managed to squash that but my commit for fixing the unsigned -> unsigned int for some reason I can't squash.
| @@ -18,6 +18,13 @@ Audio samples in WAV & QOA format can be found at: https://qoaformat.org/samples | |||
| wearing headphones. You may unexpectedly produce garbage output that can damage | |||
| your ears. I had more than a few close calls. | |||
|
|
|||
| ## Building | |||
There was a problem hiding this comment.
This commit adding cmake seems overkill for this project when for a while we didn't even need a Makefile.
This doesn't need a second parallel build system when the first is already overkill.
There was a problem hiding this comment.
Take a look at what the CMakeLists.txt does, It brings in everything automatically if it doesn't exist and marks the include directories appropriately. My main goal was to have it be accessible for most people instead of grabbing files off of a browser and pasting them off of GitHub. On top of that, more people are familiar with CMake. If you want to continue maintaining the Makefile you can do so.
qoa.h
Outdated
| @@ -366,7 +366,7 @@ unsigned int qoa_encode_frame(const short *sample_data, qoa_desc *qoa, unsigned | |||
| ), bytes, &p); | |||
|
|
|||
|
|
|||
| for (int c = 0; c < channels; c++) { | |||
| for (unsigned c = 0; c < channels; c++) { | |||
There was a problem hiding this comment.
Maybe keep these unsigned int to match channels, not just a bare unsigned.
|
|
||
| for (int c = 0; c < channels; c++) { | ||
| qoa_lms_t best_lms; |
There was a problem hiding this comment.
why don't you just do = {0}?
I also wouldn't move where this is declared.
|
|
||
| for (int c = 0; c < channels; c++) { | ||
| qoa_lms_t best_lms; | ||
| memset(&best_lms, 0, sizeof(best_lms)); |
There was a problem hiding this comment.
memset is a big hammer to suddently spring for a warning about something that will never happen. What happens when someone wants to run this library on embedded and there is no memset?
No description provided.