diff mbox series

[FFmpeg-devel,4/6] avutil/bprint: use AV_BPRINT_SIZE_AUTOMATIC instead of 1

Message ID 1593132446-21202-4-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/6] avcodec/dvbsubdec: simplify code by using OFFSET() macro
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang June 26, 2020, 12:47 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/bprint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas George June 26, 2020, 5:17 a.m. UTC | #1
lance.lmwang@gmail.com (12020-06-26):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/bprint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

No to gratuitous changes that pollute the log for no benefit:
AV_BPRINT_SIZE_AUTOMATIC = 1 is documented and part of the API.

Regards,
Soft Works June 26, 2020, 5:33 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Friday, June 26, 2020 7:17 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: Limin Wang <lance.lmwang@gmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH 4/6] avutil/bprint: use
> AV_BPRINT_SIZE_AUTOMATIC instead of 1
> 
> lance.lmwang@gmail.com (12020-06-26):
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavutil/bprint.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> No to gratuitous changes that pollute the log for no benefit:
> AV_BPRINT_SIZE_AUTOMATIC = 1 is documented and part of the API.

Isn't it a clear benefit to have a named constant where the name
of the constant indicates a meaning while a plain number does not?

softworkz
Limin Wang June 26, 2020, 10:39 a.m. UTC | #3
On Fri, Jun 26, 2020 at 07:17:01AM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-06-26):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavutil/bprint.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> No to gratuitous changes that pollute the log for no benefit:
> AV_BPRINT_SIZE_AUTOMATIC = 1 is documented and part of the API.

Sorry, I'm not clear why not to use 1 directly as it's public API?


> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George June 26, 2020, 10:41 a.m. UTC | #4
lance.lmwang@gmail.com (12020-06-26):
> Sorry, I'm not clear why not to use 1 directly as it's public API?

That is exactly what I am telling: leave it.

Regards,
Nicolas George June 26, 2020, 10:46 a.m. UTC | #5
Soft Works (12020-06-26):
> Isn't it a clear benefit to have a named constant where the name
> of the constant indicates a meaning while a plain number does not?

No. If you know the API enough to use it properly, then the meaning of 1
is obvious. If you don't, the meaning of the constant is obscure.

"Avoid magic constants" is not an absolute commandment to apply with
dogmatism, it is a rule of thumb to apply with intelligence. Like the
ban of gotos.

If this was new code, then maybe it would be slightly better to use the
named constant. But so slightly that the time you wasted just writing
this mail is enough to nullify it. And this is not new code.

Seriously, stop wasting time on useless pseudo-cosmetic changes.
Building ffmpeg.c produces a full page of warnings. These are a few
orders of magnitude more annoying than a magic 1.

Regards,
Limin Wang June 26, 2020, 10:58 a.m. UTC | #6
On Fri, Jun 26, 2020 at 12:41:19PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-06-26):
> > Sorry, I'm not clear why not to use 1 directly as it's public API?
> 
> That is exactly what I am telling: leave it.

thanks for comments, please ignore the change then.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Soft Works June 26, 2020, 1:10 p.m. UTC | #7
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Friday, June 26, 2020 12:47 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/6] avutil/bprint: use
> AV_BPRINT_SIZE_AUTOMATIC instead of 1
> 
> Soft Works (12020-06-26):
> > Isn't it a clear benefit to have a named constant where the name of
> > the constant indicates a meaning while a plain number does not?
> 
> No. If you know the API enough to use it properly, then the meaning of 1 is
> obvious. If you don't, the meaning of the constant is obscure.
> 
> "Avoid magic constants" is not an absolute commandment to apply with
> dogmatism, it is a rule of thumb to apply with intelligence. Like the ban of
> gotos.
> 
> If this was new code, then maybe it would be slightly better to use the
> named constant. But so slightly that the time you wasted just writing this mail
> is enough to nullify it. And this is not new code.
> 
> Seriously, stop wasting time on useless pseudo-cosmetic changes.
> Building ffmpeg.c produces a full page of warnings. These are a few orders of
> magnitude more annoying than a magic 1.

Hi Nicolas,

I replied because ffmpeg code is just one out of many projects where I'm doing
some work occasionally and finding such 'naked numbers' in the code is a 
regular annoyance when you have to look it up to understand the code.
I want to hint that there's another perspective, and as such, I hope that 
my writing is not a total waste.

Isn't there a paradox anyway?

When the reason to object a commit is considering it unnecessary and 
wasted time - why spend time to object and cause the developer to spend 
additional time to change the commit?  Obviously, reverting the commit 
does not recover or save any time.

I understand when someone does not want to spend his time for replacing
numbers with constant definitions. But why prevent somebody else from
doing?

As an occasional (code-)user, I'd like to state that I welcome such changes.

Kind regards,
softworkz
diff mbox series

Patch

diff --git a/libavutil/bprint.c b/libavutil/bprint.c
index 2f059c5..148b7bb 100644
--- a/libavutil/bprint.c
+++ b/libavutil/bprint.c
@@ -71,7 +71,7 @@  void av_bprint_init(AVBPrint *buf, unsigned size_init, unsigned size_max)
     unsigned size_auto = (char *)buf + sizeof(*buf) -
                          buf->reserved_internal_buffer;
 
-    if (size_max == 1)
+    if (size_max == AV_BPRINT_SIZE_AUTOMATIC)
         size_max = size_auto;
     buf->str      = buf->reserved_internal_buffer;
     buf->len      = 0;