| Message ID | 20220709043142.2228184-1-amirmazz@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Make implicit void pointer cast explicit | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| yinshiyou/commit_msg_loongarch64 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/commit_msg_armv7_RPi4 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Ping On Fri, Jul 8, 2022 at 9:31 PM Amir Mazzarella <amirmazz@google.com> wrote: > Signed-off-by: Amir Mazzarella <amirmazz@google.com> > --- > libavutil/vulkan_loader.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/vulkan_loader.h b/libavutil/vulkan_loader.h > index 3f1ee6aa46..fa8e5ed171 100644 > --- a/libavutil/vulkan_loader.h > +++ b/libavutil/vulkan_loader.h > @@ -82,7 +82,7 @@ static inline int ff_vk_load_functions(AVHWDeviceContext > *ctx, > uint64_t extensions_mask, > int has_inst, int has_dev) > { > - AVVulkanDeviceContext *hwctx = ctx->hwctx; > + AVVulkanDeviceContext *hwctx = (AVVulkanDeviceContext *) ctx->hwctx; > > static const struct FunctionLoadInfo { > int req_inst; > -- > 2.37.0.rc0.161.g10f37bed90-goog > >
Hi. Thanks for the patch. Amir Mazzarella (12022-07-09): > Signed-off-by: Amir Mazzarella <amirmazz@google.com> > --- > libavutil/vulkan_loader.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/vulkan_loader.h b/libavutil/vulkan_loader.h > index 3f1ee6aa46..fa8e5ed171 100644 > --- a/libavutil/vulkan_loader.h > +++ b/libavutil/vulkan_loader.h > @@ -82,7 +82,7 @@ static inline int ff_vk_load_functions(AVHWDeviceContext *ctx, > uint64_t extensions_mask, > int has_inst, int has_dev) > { > - AVVulkanDeviceContext *hwctx = ctx->hwctx; > + AVVulkanDeviceContext *hwctx = (AVVulkanDeviceContext *) ctx->hwctx; > > static const struct FunctionLoadInfo { > int req_inst; Why? What is it supposed to fix? Regards,
Thank you for your response! A C++ compiler can't do implicit casts like a C compiler can, in this instance. This is fine for most of FFmpeg's codebase, since these tricks are in C source files, but in this instance it is in a header file. If any C++ code includes this header file, even with extern "C", it won't be able to be compiled. On Tue, Jul 19, 2022 at 12:08 PM Nicolas George <george@nsup.org> wrote: > Hi. Thanks for the patch. > > Amir Mazzarella (12022-07-09): > > Signed-off-by: Amir Mazzarella <amirmazz@google.com> > > --- > > libavutil/vulkan_loader.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavutil/vulkan_loader.h b/libavutil/vulkan_loader.h > > index 3f1ee6aa46..fa8e5ed171 100644 > > --- a/libavutil/vulkan_loader.h > > +++ b/libavutil/vulkan_loader.h > > @@ -82,7 +82,7 @@ static inline int > ff_vk_load_functions(AVHWDeviceContext *ctx, > > uint64_t extensions_mask, > > int has_inst, int has_dev) > > { > > - AVVulkanDeviceContext *hwctx = ctx->hwctx; > > + AVVulkanDeviceContext *hwctx = (AVVulkanDeviceContext *) ctx->hwctx; > > > > static const struct FunctionLoadInfo { > > int req_inst; > > Why? What is it supposed to fix? > > Regards, > > -- > Nicolas George >
On Tue, Jul 19, 2022 at 11:21 PM Amir Mazzarella <amirmazz-at-google.com@ffmpeg.org> wrote: > > Thank you for your response! A C++ compiler can't do implicit casts like a > C compiler can, in this instance. This is fine for most of FFmpeg's > codebase, since these tricks are in C source files, but in this instance it > is in a header file. If any C++ code includes this header file, even with > extern "C", it won't be able to be compiled. > While this is true, extremely little code in FFmpeg is C++, as it is avoided unless absolutely necessary. We're more likely to fix such cases when needed, rather than blanket them "just in case". - Hendrik
Amir Mazzarella (12022-07-19): > Thank you for your response! A C++ compiler can't do implicit casts like a > C compiler can, in this instance. This is fine for most of FFmpeg's > codebase, since these tricks are in C source files, but in this instance it > is in a header file. If any C++ code includes this header file, even with > extern "C", it won't be able to be compiled. Thanks for clarifying. I confess I had more or less guessed. FFmpeg is C, and its headers, public or not, as a rule do not contain accommodations to be included as is in other languages. > On Tue, Jul 19, 2022 at 12:08 PM Nicolas George <george@nsup.org> wrote: Please remember that top-posting is forbidden on this mailing-list. If you do not know what it means, look it up. Regards,
On Tue, Jul 19, 2022 at 2:24 PM Nicolas George <george@nsup.org> wrote: > Thanks for clarifying. I confess I had more or less guessed. FFmpeg is > C, and its headers, public or not, as a rule do not contain > accommodations to be included as is in other languages. I understand that FFmpeg does not contain accommodations, but in my opinion it is less of an accommodation and more for readability, avoiding an implicit cast. An explicit void pointer cast, like the one presented in my patch, is done elsewhere in FFmpeg code as well. See https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/hwcontext_qsv.c#L316 What's the harm in including this one too?
Amir Mazzarella (12022-07-19): > I understand that FFmpeg does not contain accommodations, but in my > opinion it is less of an accommodation and more for readability, avoiding > an implicit cast. An explicit void pointer cast, like the one presented in The arguments against useless casts have been worded a thousand times, let us not waste everybody's energy with one more. > my patch, is done elsewhere in FFmpeg code as well. > See https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/hwcontext_qsv.c#L316 It is not the only coding style inconsistency around this line. Coding style inconsistencies cannot be considered precedents. Regards,
Quoting Amir Mazzarella (2022-07-19 23:21:00) > Thank you for your response! A C++ compiler can't do implicit casts like a > C compiler can, in this instance. This is fine for most of FFmpeg's > codebase, since these tricks are in C source files, but in this instance it > is in a header file. If any C++ code includes this header file, even with > extern "C", it won't be able to be compiled. This header is private, no external c++ code should be including it.
diff --git a/libavutil/vulkan_loader.h b/libavutil/vulkan_loader.h index 3f1ee6aa46..fa8e5ed171 100644 --- a/libavutil/vulkan_loader.h +++ b/libavutil/vulkan_loader.h @@ -82,7 +82,7 @@ static inline int ff_vk_load_functions(AVHWDeviceContext *ctx, uint64_t extensions_mask, int has_inst, int has_dev) { - AVVulkanDeviceContext *hwctx = ctx->hwctx; + AVVulkanDeviceContext *hwctx = (AVVulkanDeviceContext *) ctx->hwctx; static const struct FunctionLoadInfo { int req_inst;
Signed-off-by: Amir Mazzarella <amirmazz@google.com> --- libavutil/vulkan_loader.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)