Message ID | 1584505422-21835-1-git-send-email-linjie.fu@intel.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] lavf/protocols: Fix compile warning of discarding 'const' qualifier from pointer target type | expand |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | success | Make fate finished |
Linjie Fu: > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > libavformat/protocols.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavformat/protocols.c b/libavformat/protocols.c > index f1b8eab..cb19f77 100644 > --- a/libavformat/protocols.c > +++ b/libavformat/protocols.c > @@ -95,9 +95,9 @@ const AVClass *ff_urlcontext_child_class_next(const AVClass *prev) > > const char *avio_enum_protocols(void **opaque, int output) > { > - const URLProtocol **p = *opaque; > + URLProtocol **p = *opaque; > > - p = p ? p + 1 : url_protocols; > + p = p ? p + 1 : (URLProtocol **)url_protocols; > *opaque = p; > if (!*p) { > *opaque = NULL; > 1. The actual problem with this code is not that the URLProtocols are const (which they are), but that the pointers in the url_protocols array are constant pointers (to const URLProtocol); yet the way things are right now the user gets a non-const pointer to these const pointers (to const URLProtocol). 2. It follows from this that changing the type of p in the way you did it is actually unnecessary (if one wanted to solve the problem by casting const away, you could cast url_protocols to (const URLProtocol **) (from const URLProtocol * const *)) and a step in the wrong direction (given that the underlying URLProtocol is const). 3. Casting const away is generally a sign of bad design and this is no exception. The signature is wrong, the opaque should be const void**, so that the user never gets a chance to get a pointer to nonconst to something that is actually const. 4. See [1] for an approach that fixes the underlying issue. - Andreas [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/248675.html
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Andreas Rheinhardt > Sent: Wednesday, March 18, 2020 12:52 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] lavf/protocols: Fix compile warning of > discarding 'const' qualifier from pointer target type > > Linjie Fu: > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > --- > > libavformat/protocols.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/protocols.c b/libavformat/protocols.c > > index f1b8eab..cb19f77 100644 > > --- a/libavformat/protocols.c > > +++ b/libavformat/protocols.c > > @@ -95,9 +95,9 @@ const AVClass *ff_urlcontext_child_class_next(const > AVClass *prev) > > > > const char *avio_enum_protocols(void **opaque, int output) > > { > > - const URLProtocol **p = *opaque; > > + URLProtocol **p = *opaque; > > > > - p = p ? p + 1 : url_protocols; > > + p = p ? p + 1 : (URLProtocol **)url_protocols; > > *opaque = p; > > if (!*p) { > > *opaque = NULL; > > > 1. The actual problem with this code is not that the URLProtocols are > const (which they are), but that the pointers in the url_protocols array > are constant pointers (to const URLProtocol); yet the way things are > right now the user gets a non-const pointer to these const pointers (to > const URLProtocol). Yes, a "const" is needed for "void ** opaque" since it's a pointer to a pointer to a const type. > 2. It follows from this that changing the type of p in the way you did > it is actually unnecessary (if one wanted to solve the problem by > casting const away, you could cast url_protocols to (const URLProtocol > **) (from const URLProtocol * const *)) and a step in the wrong > direction (given that the underlying URLProtocol is const). You are right. > 3. Casting const away is generally a sign of bad design and this is no > exception. The signature is wrong, the opaque should be const void**, so > that the user never gets a chance to get a pointer to nonconst to > something that is actually const. > > 4. See [1] for an approach that fixes the underlying issue. > > - Andreas > > [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/248675.html Thanks for the elaborations, the fix in [1] is more reasonable. However got one concern while verifying the patch, replied under your thread. - Linjie
diff --git a/libavformat/protocols.c b/libavformat/protocols.c index f1b8eab..cb19f77 100644 --- a/libavformat/protocols.c +++ b/libavformat/protocols.c @@ -95,9 +95,9 @@ const AVClass *ff_urlcontext_child_class_next(const AVClass *prev) const char *avio_enum_protocols(void **opaque, int output) { - const URLProtocol **p = *opaque; + URLProtocol **p = *opaque; - p = p ? p + 1 : url_protocols; + p = p ? p + 1 : (URLProtocol **)url_protocols; *opaque = p; if (!*p) { *opaque = NULL;
Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- libavformat/protocols.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)