On Wed, Dec 17, 2014 at 3:53 PM, Martin Storsjö <[email protected]> wrote: > On Wed, 17 Dec 2014, Vittorio Giovara wrote: > >> On Wed, Dec 17, 2014 at 3:31 PM, Martin Storsjö <[email protected]> wrote: >>> >>> On Wed, 17 Dec 2014, Vittorio Giovara wrote: >>> >>>> CC: [email protected] >>>> Bug-Id: CID 732285 / CID 732286 >>>> --- >>>> libavcodec/aacps.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavcodec/aacps.c b/libavcodec/aacps.c >>>> index 8f55c7f..df069c3 100644 >>>> --- a/libavcodec/aacps.c >>>> +++ b/libavcodec/aacps.c >>>> @@ -139,7 +139,7 @@ static int ps_read_extension_data(GetBitContext *gb, >>>> PSContext *ps, int ps_exten >>>> return get_bits_count(gb) - count; >>>> } >>>> >>>> -static void ipdopd_reset(int8_t *opd_hist, int8_t *ipd_hist) >>>> +static void ipdopd_reset(int8_t *ipd_hist, int8_t *opd_hist) >>>> { >>>> int i; >>>> for (i = 0; i < PS_MAX_NR_IPDOPD; i++) { >>>> -- >>>> 1.9.3 (Apple Git-50) >>> >>> >>> >>> The commit message needs expanding - on its own this change doesn't look >>> like it makes much sense, unless you say e.g. that this is the order that >>> the caller expects it to be, or whatever the issue is. >>> >>> In general, unless it's absolutely crystal clear what issue coverity is >>> complaining about, please mention it in the commit message, don't just >>> mention an opaque CID. (Remember, commit messages should make sense on >>> their >>> own and external references should just be an extra; all the vital >>> information shouldn't be in the external references.) Without reading the >>> rest of the file, it's impossible to judge from just reading this one >>> single >>> patch what it is about. >>> >>> Also, the things you swap are parameters, not operands. The comma >>> inbetween >>> is not the comma operator. >> >> >> You are right. Would something like the following be better instead? >> >> aacps: invert the order of parameters of ipdopd_reset() >> >> This is the order that the caller uses in the rest of the file. >> >> Bug-Id [...] > > > That makes it better. It'd be nice to have info about the severity as well - > is it only a cosmetic issue (e.g. the function does the same thing to both > parameters), or does this fix some actual behaviour issue? (I.e., what > strikes me as a reader of the patch, for such a potentially fatal bug, is, > how come this hasn't shown up as an issue before somewhere?)
The function itself applies the same operation to both parameter, so this instance is purely cosmetic: would it be enough to mention that with a proper tag in the log? eg aacps: cosmetics: invert the order of parameters of ipdopd_reset() -- Vittorio _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
