On Sat, 28 Jan 2017, Michael Niedermayer wrote:
On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote:
On 27.01.2017 02:56, Marton Balint wrote:
I see 3 problems (wm4 explicitly named them, but I also had them in mind)
- Little benefit, yet
- Makes the code less clean, more cluttered
- Increases binary size
The ideas I proposed (use macros, use common / factorized checks for common
validatons and errors) might be a good compromise IMHO. Fuzzing thereforce
can be done with "debug" builds.
Anyway, I am not blocking the patch, just expressing what I would prefer in
the long run.
How about the following macro?
#define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \
if (condition) { \
if (!CONFIG_SMALL && log_ctx) \
av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \
return error; \
} \
}
That could be used with error message:
FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS,
AVERROR(ENOSYS),
s, "Too many channels %d > %d\n", st->codecpar->channels,
FF_SANE_NB_CHANNELS)
Or without:
FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS,
AVERROR(ENOSYS), NULL)
I suggest
if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels,
FF_SANE_NB_CHANNELS);
return AVERROR(ENOSYS);
}
or for the 2nd example:
if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
return AVERROR(ENOSYS);
ff_elog() can then be defined to nothing based on CONFIG_SMALL
the difference between my suggestion and yours is that mine has
new-lines seperating the condition, message and return and that its
self explanatory C code.
What you do is removing newlines and standard C keywords with a custom
macro that people not working on FFmpeg on a regular basis will have
difficulty understanding
The macro is similar to writing (minus the C keywords)
if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog(
s, "Too many channels %d > %d\n", st->codecpar->channels,
FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); }
we dont do that becuause its just bad code
why does this suddenly become desirable when its in a macro?
Or maybe the question should be, does code become less cluttered and
cleaner when newlines and C keywords are removed ?
if not why is that done here ?
if yes why is it done just here ?
If we reduce the number of extra lines (not at any cost), I think that
helps. There is also a solution which keeps the traditional C syntax, and
is easy to undestand even at first glance.
if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n",
st->codecpar->channels, FF_SANE_NB_CHANNELS);
Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
