Fix compilation warnings#1711
Conversation
3af066a to
b787139
Compare
| typedef enum { _ld_fast, _ld_slow } LoadDict_mode_e; | ||
| #define HASH_UNIT sizeof(reg_t) | ||
| int LZ4_loadDict_internal(LZ4_stream_t* LZ4_dict, | ||
| static int LZ4_loadDict_internal(LZ4_stream_t* LZ4_dict, |
There was a problem hiding this comment.
Sidenote: I'm really surprised this was not caught before. We may be missing a compilation flag to catch that.
There was a problem hiding this comment.
Clearly, we are missing -Wmissing-prototypes. This should be added. I would have caught these issues.
|
Do you have a reproduction scenario, that would produce the warnings without this fix? |
| **************************************/ | ||
| #if !defined (__VMS) && (defined (__cplusplus) || (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) /* C99 */) ) | ||
| # include <stdint.h> | ||
| # ifndef XXH_PRIVATE_API |
There was a problem hiding this comment.
This one feels weird. Like a misplaced responsibility, or a fragile coupling.
But, it's not immediately obvious which better solution exists out there...
There was a problem hiding this comment.
I think some proper solutions are:
- update
xxhashto a newer version that doesn't useU32/U16(it moved toxxh_u32/xxh_u16since) - if using the xxhash version bundled in
lz4,#define MEM_MODULE, this will disable thetypedef U32part withinxxhash. - rely on
C11to maketypedefredefinition valid - silence the specific warning
There was a problem hiding this comment.
Apologies for the late reply. I found these while upgrading the optionally used LZ4 amalgamation that ships with Subversion.
This code already implements my proposed patch.
Notes:
lz4.hwas renamed tolz4internal.hto avoid header name clashes, since Subversion can be compiled with the included amalgamated LZ4 even when an external library is available.- The entry point for compiling LZ4 is
svnlz4.c. svnlz4.hdefines some Subversion-specific options, like changing the visibility of the LZ4 API.
As for relying on C11 – Subversion compiles with -std=c90 and it would make including LZ4 in our source a bit easier if C11 constructs could be avoided.
There was a problem hiding this comment.
This #ifdef isn't needed any more.
|
|
b787139 to
77aa8bf
Compare
|
I just resolved the conflicts and there's only one commit left. The changes you already made seem to work now in my environment. |
In a custom amalgamated build of LZ4 with
XXH_PRIVATE_APIdefined,-std=c90and stricter compilation warnings, three classes of warnings were emitted. This is a trivial fix with no functional change.