| Message ID | 1425034865-28812-4-git-send-email-martin@martin.st |
|---|---|
| State | Committed |
| Commit | 1fc64e2e07787bbca82a72c146588e850e6d098a |
| Headers | show |
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.
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
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
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.
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
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
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
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
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
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.
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
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
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);