On Wed, 17 Dec 2014, Vittorio Giovara wrote:
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()
No, rather say that in the commit message (adding "cosmetics" to a patch
which changes parameter orders looks highly suspicious):
---8<---
aacps: invert the order of parameters of ipdopd_reset()
This is the order that the caller uses in the rest of the file. The same
operation is applied to both parameters, so this change is only done for
consistency, it doesn't change the actual behaviour.
Bug-Id [...]
---8<---
That way, it is crystal clear to every reader of the commit what it does,
what the issue was, what impact it has (e.g. for people maintaining
security/stability relevant patches for their own branch, or for the
public release branches) - i.e. should I apply it asap everywhere
applicable and make a large effort to figure our where I'm potentially
running that code, or just treat it as a typofix?
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
