[04/12] rtpenc: Write conditional statements on separate lines

Message ID 1425034865-28812-4-git-send-email-martin@martin.st
State Committed
Commit 1fc64e2e07787bbca82a72c146588e850e6d098a
Headers show

Commit Message

Martin Storsjö Feb. 27, 2015, 11 a.m.
Intentionally keeping some conditional statements on single lines
in rtpenc_h263.c.
---
 libavformat/rtpenc.c      | 3 ++-
 libavformat/rtpenc_xiph.c | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

Comments

Luca Barbato Feb. 28, 2015, 12:25 a.m. | #1
On 27/02/15 12:00, Martin Storsjö wrote:
> Intentionally keeping some conditional statements on single lines
> in rtpenc_h263.c.
> ---
>  libavformat/rtpenc.c      | 3 ++-
>  libavformat/rtpenc_xiph.c | 6 ++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 

Ok.
Diego Biurrun March 1, 2015, 8:27 p.m. | #2
On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
> Intentionally keeping some conditional statements on single lines
> in rtpenc_h263.c.
> ---
>  libavformat/rtpenc.c      | 3 ++-
>  libavformat/rtpenc_xiph.c | 6 ++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)

How about K&Ring the whole files instead?  Yes, I'm volunteering.

Diego
Martin Storsjö March 1, 2015, 8:46 p.m. | #3
On Sun, 1 Mar 2015, Diego Biurrun wrote:

> On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
>> Intentionally keeping some conditional statements on single lines
>> in rtpenc_h263.c.
>> ---
>>  libavformat/rtpenc.c      | 3 ++-
>>  libavformat/rtpenc_xiph.c | 6 ++++--
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> How about K&Ring the whole files instead?  Yes, I'm volunteering.

I don't see a direct need. Most of these files are quite readable and 
mostly very close to our common standard.

// Martin
Anton Khirnov March 1, 2015, 9:15 p.m. | #4
Quoting Diego Biurrun (2015-03-01 21:27:30)
> On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
> > Intentionally keeping some conditional statements on single lines
> > in rtpenc_h263.c.
> > ---
> >  libavformat/rtpenc.c      | 3 ++-
> >  libavformat/rtpenc_xiph.c | 6 ++++--
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> How about K&Ring the whole files instead?  Yes, I'm volunteering.
> 

Stab.

As I've said many times before -- complete whole-file reformatting
should be a last-resort measure applied to terrible unreadable files
(like ye olde days mpegvideo). We used to have many such files, so it
seems some of use have gotten used to doing it all the time, but
it's a very bad idea to keep doing this on files that are mostly fine.
It creates unnecessary noise in the history, is hard and annoying to
review properly and breaks outstanding work.

So -- don't.
Diego Biurrun March 1, 2015, 9:19 p.m. | #5
On Sun, Mar 01, 2015 at 10:46:33PM +0200, Martin Storsjö wrote:
> On Sun, 1 Mar 2015, Diego Biurrun wrote:
> 
> >On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
> >>Intentionally keeping some conditional statements on single lines
> >>in rtpenc_h263.c.
> >>---
> >> libavformat/rtpenc.c      | 3 ++-
> >> libavformat/rtpenc_xiph.c | 6 ++++--
> >> 2 files changed, 6 insertions(+), 3 deletions(-)
> >
> >How about K&Ring the whole files instead?  Yes, I'm volunteering.
> 
> I don't see a direct need. Most of these files are quite readable and mostly
> very close to our common standard.

You're touching the file already in this respect.  Better change it once
and for all than going back and touching it at another point in the future.
Less noise in the history and all that ...

Diego
Diego Biurrun March 1, 2015, 9:21 p.m. | #6
On Sun, Mar 01, 2015 at 10:15:14PM +0100, Anton Khirnov wrote:
> Quoting Diego Biurrun (2015-03-01 21:27:30)
> > On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
> > > Intentionally keeping some conditional statements on single lines
> > > in rtpenc_h263.c.
> > > ---
> > >  libavformat/rtpenc.c      | 3 ++-
> > >  libavformat/rtpenc_xiph.c | 6 ++++--
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > How about K&Ring the whole files instead?  Yes, I'm volunteering.
> 
> Stab.
> 
> As I've said many times before -- complete whole-file reformatting
> should be a last-resort measure applied to terrible unreadable files
> (like ye olde days mpegvideo). We used to have many such files, so it
> seems some of use have gotten used to doing it all the time, but
> it's a very bad idea to keep doing this on files that are mostly fine.
> It creates unnecessary noise in the history, is hard and annoying to
> review properly and breaks outstanding work.

And that's precisely why I'm proposing to do it once (and for all) instead
of changing little bits here and there, thus littering the history with
cleanup commits...

> So -- don't.

So -- stab.  :)

Diego
Martin Storsjö March 1, 2015, 9:22 p.m. | #7
On Sun, 1 Mar 2015, Diego Biurrun wrote:

> On Sun, Mar 01, 2015 at 10:46:33PM +0200, Martin Storsjö wrote:
>> On Sun, 1 Mar 2015, Diego Biurrun wrote:
>>
>>> On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
>>>> Intentionally keeping some conditional statements on single lines
>>>> in rtpenc_h263.c.
>>>> ---
>>>> libavformat/rtpenc.c      | 3 ++-
>>>> libavformat/rtpenc_xiph.c | 6 ++++--
>>>> 2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> How about K&Ring the whole files instead?  Yes, I'm volunteering.
>>
>> I don't see a direct need. Most of these files are quite readable and mostly
>> very close to our common standard.
>
> You're touching the file already in this respect.

Yes.

> Better change it once and for all than going back and touching it at 
> another point in the future. Less noise in the history and all that ...

No.

Yes, I'm touching the file. I'm touching very small and easy to read 
sections. Such a touch is less costly than touching every single line in 
the file to me, and to people reviewing it, and to people reviewing the 
history of the file, and to people cherrypicking fixes back and forth.

The file is mostly fine and does not warrant a huge monster patch.

Yes, there might actually be a few "if(" or "switch(" there. But don't you 
dare send a patch that does any insignificant vertical alignment or other 
completely irrelevant shuffling.

// Martin
Martin Storsjö March 1, 2015, 9:24 p.m. | #8
On Sun, 1 Mar 2015, Diego Biurrun wrote:

> On Sun, Mar 01, 2015 at 10:15:14PM +0100, Anton Khirnov wrote:
>> Quoting Diego Biurrun (2015-03-01 21:27:30)
>>> On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
>>>> Intentionally keeping some conditional statements on single lines
>>>> in rtpenc_h263.c.
>>>> ---
>>>>  libavformat/rtpenc.c      | 3 ++-
>>>>  libavformat/rtpenc_xiph.c | 6 ++++--
>>>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> How about K&Ring the whole files instead?  Yes, I'm volunteering.
>>
>> Stab.
>>
>> As I've said many times before -- complete whole-file reformatting
>> should be a last-resort measure applied to terrible unreadable files
>> (like ye olde days mpegvideo). We used to have many such files, so it
>> seems some of use have gotten used to doing it all the time, but
>> it's a very bad idea to keep doing this on files that are mostly fine.
>> It creates unnecessary noise in the history, is hard and annoying to
>> review properly and breaks outstanding work.
>
> And that's precisely why I'm proposing to do it once (and for all) instead
> of changing little bits here and there, thus littering the history with
> cleanup commits...

"Once and for all", until 6 months later when somebody else comes along 
and redoes with slightly different uncrustify/whatever tool config, and 
then someone else comes along again? You really don't see that this is an 
issue, do you?

// Martin
Martin Storsjö March 1, 2015, 9:25 p.m. | #9
On Sun, 1 Mar 2015, Diego Biurrun wrote:

> And that's precisely why I'm proposing to do it once (and for all) 
> instead of changing little bits here and there, thus littering the 
> history with cleanup commits...

Also, to me, littering a file's history with one single full rewrite of 
every single line is much much much more annoying, than having 3-4 small 
cosmetic cleanup commits (instead of 1 huge one, which is impossible to 
review).

// Martin
Anton Khirnov March 1, 2015, 9:58 p.m. | #10
Quoting Diego Biurrun (2015-03-01 22:21:07)
> On Sun, Mar 01, 2015 at 10:15:14PM +0100, Anton Khirnov wrote:
> > Quoting Diego Biurrun (2015-03-01 21:27:30)
> > > On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
> > > > Intentionally keeping some conditional statements on single lines
> > > > in rtpenc_h263.c.
> > > > ---
> > > >  libavformat/rtpenc.c      | 3 ++-
> > > >  libavformat/rtpenc_xiph.c | 6 ++++--
> > > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > How about K&Ring the whole files instead?  Yes, I'm volunteering.
> > 
> > Stab.
> > 
> > As I've said many times before -- complete whole-file reformatting
> > should be a last-resort measure applied to terrible unreadable files
> > (like ye olde days mpegvideo). We used to have many such files, so it
> > seems some of use have gotten used to doing it all the time, but
> > it's a very bad idea to keep doing this on files that are mostly fine.
> > It creates unnecessary noise in the history, is hard and annoying to
> > review properly and breaks outstanding work.
> 
> And that's precisely why I'm proposing to do it once (and for all) instead
> of changing little bits here and there, thus littering the history with
> cleanup commits...

You're disregarding the fact that the file in question is for the most
part perfectly fine.

And as Martin also said, we do not have strict rules on all aspects of
formatting (nor should we). So there is no "once and for all", different
people will leave it in a different state after reformatting.
Diego Biurrun March 1, 2015, 10 p.m. | #11
On Sun, Mar 01, 2015 at 11:24:18PM +0200, Martin Storsjö wrote:
> On Sun, 1 Mar 2015, Diego Biurrun wrote:
> >On Sun, Mar 01, 2015 at 10:15:14PM +0100, Anton Khirnov wrote:
> >>Quoting Diego Biurrun (2015-03-01 21:27:30)
> >>>On Fri, Feb 27, 2015 at 01:00:57PM +0200, Martin Storsjö wrote:
> >>>>Intentionally keeping some conditional statements on single lines
> >>>>in rtpenc_h263.c.
> >>>>---
> >>>> libavformat/rtpenc.c      | 3 ++-
> >>>> libavformat/rtpenc_xiph.c | 6 ++++--
> >>>> 2 files changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>>How about K&Ring the whole files instead?  Yes, I'm volunteering.
> >>
> >>Stab.
> >>
> >>As I've said many times before -- complete whole-file reformatting
> >>should be a last-resort measure applied to terrible unreadable files
> >>(like ye olde days mpegvideo). We used to have many such files, so it
> >>seems some of use have gotten used to doing it all the time, but
> >>it's a very bad idea to keep doing this on files that are mostly fine.
> >>It creates unnecessary noise in the history, is hard and annoying to
> >>review properly and breaks outstanding work.
> >
> >And that's precisely why I'm proposing to do it once (and for all) instead
> >of changing little bits here and there, thus littering the history with
> >cleanup commits...
> 
> "Once and for all", until 6 months later when somebody else comes along and
> redoes with slightly different uncrustify/whatever tool config, and then
> someone else comes along again? You really don't see that this is an issue,
> do you?

I very much agree that this would be an issue, but I have never seen it
happen, nor do I expect it to ever happen.  K&R style is fixed and our
uncrustify config is pretty stable.

Diego
Martin Storsjö March 1, 2015, 10:07 p.m. | #12
On Sun, 1 Mar 2015, Diego Biurrun wrote:

> On Sun, Mar 01, 2015 at 11:24:18PM +0200, Martin Storsjö wrote:
>>
>> "Once and for all", until 6 months later when somebody else comes along and
>> redoes with slightly different uncrustify/whatever tool config, and then
>> someone else comes along again? You really don't see that this is an issue,
>> do you?
>
> I very much agree that this would be an issue, but I have never seen it
> happen, nor do I expect it to ever happen.  K&R style is fixed and our
> uncrustify config is pretty stable.

There are a number of cases of this actually happening (I don't have them 
here right now but I do remember you being shown some of them). Also, 
_your_ uncrustify config perhaps is pretty stable, but that doesn't seem 
to have helped so far. I hope you see the flaw in the assumption that 
everybody are abiding to your uncrustify config (note: Anton pointed out 
that we don't have, and shouldn't have, such a tightly formally written 
spec on what code should look like).

// Martin

Patch

diff --git a/libavformat/rtpenc.c b/libavformat/rtpenc.c
index b306a4c..cfa5f34 100644
--- a/libavformat/rtpenc.c
+++ b/libavformat/rtpenc.c
@@ -220,7 +220,8 @@  static int rtp_write_header(AVFormatContext *s1)
         break;
     case AV_CODEC_ID_VORBIS:
     case AV_CODEC_ID_THEORA:
-        if (!s->max_frames_per_packet) s->max_frames_per_packet = 15;
+        if (!s->max_frames_per_packet)
+            s->max_frames_per_packet = 15;
         s->max_frames_per_packet = av_clip(s->max_frames_per_packet, 1, 15);
         s->max_payload_size -= 6; // ident+frag+tdt/vdt+pkt_num+pkt_length
         s->num_frames = 0;
diff --git a/libavformat/rtpenc_xiph.c b/libavformat/rtpenc_xiph.c
index 07086b1..def3bc5 100644
--- a/libavformat/rtpenc_xiph.c
+++ b/libavformat/rtpenc_xiph.c
@@ -81,14 +81,16 @@  void ff_rtp_send_xiph(AVFormatContext *s1, const uint8_t *buff, int size)
         }
 
         // buffer current frame to send later
-        if (0 == s->num_frames) s->timestamp = s->cur_timestamp;
+        if (0 == s->num_frames)
+            s->timestamp = s->cur_timestamp;
         s->num_frames++;
 
         // Set packet header. Normally, this is OR'd with frag and xdt,
         // but those are zero, so omitted here
         *q++ = s->num_frames;
 
-        if (s->num_frames > 1) q = s->buf_ptr; // jump ahead if needed
+        if (s->num_frames > 1)
+            q = s->buf_ptr; // jump ahead if needed
         *q++ = (size >> 8) & 0xff;
         *q++ = size & 0xff;
         memcpy(q, buff, size);