Hi baziotis! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

please sign at https://code.facebook.com/cla.

I just did that, I hope it's fine.

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Looks like there are some CI issues on appveyor:)

Looks like there are some CI issues on appveyor:)

Thank you, I saw that but I'm not sure why that is. It's weird because it's the only one failing. Maybe the MSVC version does not support a C feature I used. I don't have a Windows machine at hand but I'll try to find one.

Ok, I found some time to test on a Windows machine. Here's a somewhat small example that can recreate the bug:

#include <stdio.h>  // size_t

typedef enum {
  ZSTD_error_checksum_wrong      = 22,
} ZSTD_ErrorCode;

#define RAWLOG(...)
#define PREFIX(name) ZSTD_error_##name
#define ZSTD_ERROR(name) ((size_t)-PREFIX(name))
#define ERROR(name) ZSTD_ERROR(name)

#define ZSTD_QUOTE(str) #str

#define _REDUCE_MACRO(\
    _1, _2, _3, _4, \
    _5, _6, _7, _8, \
    _9, _10, _11, _12, \
    _13, _14, _15, _16, ...) _16

#define RETURN_ERROR_IF2(cond, err) \
  if (cond) { \
    RAWLOG(3, "%s:%d: ERROR!: check %s failed, returning %s", __FILE__, __LINE__, ZSTD_QUOTE(cond), ZSTD_QUOTE(ERROR(err))); \
    return ERROR(err); \
  }

#define RETURN_ERROR_IF_MORE(cond, err, ...) \
  if (cond) { \
    RAWLOG(3, "%s:%d: ERROR!: check %s failed, returning %s", __FILE__, __LINE__, ZSTD_QUOTE(cond), ZSTD_QUOTE(ERROR(err))); \
    RAWLOG(3, ": " __VA_ARGS__); \
    RAWLOG(3, "\n"); \
    return ERROR(err); \
  }

#define _RM_RET_IF(c, e, ...) RETURN_ERROR_IF_MORE(c, e, __VA_ARGS__)

#define RETURN_ERROR_IF(...) \
    _REDUCE_MACRO(__VA_ARGS__, \
        _RM_RET_IF, _RM_RET_IF, _RM_RET_IF, _RM_RET_IF, \
        _RM_RET_IF, _RM_RET_IF, _RM_RET_IF, _RM_RET_IF, \
        _RM_RET_IF, _RM_RET_IF, _RM_RET_IF, _RM_RET_IF, \
        _RM_RET_IF, RETURN_ERROR_IF2,)(__VA_ARGS__)

int main(void) {
    RETURN_ERROR_IF(0, checksum_wrong);
}

I probably have used some GCC specific feature but I can't see where. I'll try to check again tomorrow.

Btw, is the - in front of PREFIX in ZSTD_ERROR() correct (it's the original) ? Because the error codes have positive values.

I forgot to add yesterday: MSVC doesn't seem to have the equivalent of pedantic, so we can just ignore it, which is the better choice anyway. It has /Wall as the highest warning level and /WX which is treat all warnings as errors. Combining both of those does not give an error so we should be ok.

Ping :)

This topic is a bit difficult.

Do I understand correctly that the main issue is to silence some warnings about invoking some variadic macros with zero argument (mostly RETURN_ERROR_IF(), sometimes FORWARD_IF_ERROR()) when compiled with -pedantic ?

The PR does the job. And reaching 0-warning at -pedantic level is a rather good thing.
Another good property is that modifications are well located, the PR only modifies zstd_internal.h.

It's a vague feeling, but the changes introduced seem intuitively too heavy for the problem at hand. And it becomes a question of maintenance : do we want to maintain that code ? Can we read and modify it easily ? Does it feel clear enough ? Do we fully understand its potential side effects ?

I may be just me, but when I read the code, I don't have the feeling that I "fully get it". I can read the comment (they are clear, thanks), I can imply what it does, but it still feels, well, hacky. I can't anticipate what kind of side effects this hack may introduce.

I'm not sure if there is a better solution.
That being said, when it comes to -pedantic level, it wouldn't be the first time that I would consider silencing a specific warning that feels to complex to fix (in this case, -Wno-gnu-zero-variadic-macro-arguments).

@Cyan4973 I agree with your comment very much. I probably have mentioned in some comment somewhere that I am not particularly proud of the solution, but it was the best I could (note that even finding a solution was difficult). And unfortunately, from what I know regarding the preprocessor, I don't think there's a simpler one.
In any case, no problem if you close this.

Yeah, I looked around too, but could not find any reasonable solution with mild complexity.
Faced with this choice, I believe I would prefer turning off the specific warning.
So let's close it for now.

Thanks for trying !

No problem.