| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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,
> -----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
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
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,
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,
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".
> -----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 --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;