| Message ID | GV1P250MB0737B801E6E8032FF87A5B108F362@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/10] doc/examples: Always use <> includes | expand |
| Context | Check | Description |
|---|---|---|
| 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 |
On Mon, Mar 25, 2024 at 12:38 PM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavutil/avstring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/avstring.c b/libavutil/avstring.c > index 2071dd36a5..8702fe0455 100644 > --- a/libavutil/avstring.c > +++ b/libavutil/avstring.c > @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path, const > char *component) > char *fullpath; > > if (!path) > - return av_strdup(component); > + return component ? av_strdup(component) : NULL; > if (!component) > return av_strdup(path); > isn't this what av_strdup already does?
Vittorio Giovara: > On Mon, Mar 25, 2024 at 12:38 PM Andreas Rheinhardt < > andreas.rheinhardt@outlook.com> wrote: > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavutil/avstring.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavutil/avstring.c b/libavutil/avstring.c >> index 2071dd36a5..8702fe0455 100644 >> --- a/libavutil/avstring.c >> +++ b/libavutil/avstring.c >> @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path, const >> char *component) >> char *fullpath; >> >> if (!path) >> - return av_strdup(component); >> + return component ? av_strdup(component) : NULL; >> if (!component) >> return av_strdup(path); >> > > isn't this what av_strdup already does? It's not documented to do so. It could also decide to treat av_strdup(NULL) as av_strdup(""). - Andreas
On Mon, Mar 25, 2024 at 12:55 PM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Vittorio Giovara: > > On Mon, Mar 25, 2024 at 12:38 PM Andreas Rheinhardt < > > andreas.rheinhardt@outlook.com> wrote: > > > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >> --- > >> libavutil/avstring.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavutil/avstring.c b/libavutil/avstring.c > >> index 2071dd36a5..8702fe0455 100644 > >> --- a/libavutil/avstring.c > >> +++ b/libavutil/avstring.c > >> @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path, > const > >> char *component) > >> char *fullpath; > >> > >> if (!path) > >> - return av_strdup(component); > >> + return component ? av_strdup(component) : NULL; > >> if (!component) > >> return av_strdup(path); > >> > > > > isn't this what av_strdup already does? > > It's not documented to do so. It could also decide to treat > av_strdup(NULL) as av_strdup(""). > Ah fair point, but should we not update its documentation instead?
On 3/25/2024 1:55 PM, Andreas Rheinhardt wrote: > Vittorio Giovara: >> On Mon, Mar 25, 2024 at 12:38 PM Andreas Rheinhardt < >> andreas.rheinhardt@outlook.com> wrote: >> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavutil/avstring.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavutil/avstring.c b/libavutil/avstring.c >>> index 2071dd36a5..8702fe0455 100644 >>> --- a/libavutil/avstring.c >>> +++ b/libavutil/avstring.c >>> @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path, const >>> char *component) >>> char *fullpath; >>> >>> if (!path) >>> - return av_strdup(component); >>> + return component ? av_strdup(component) : NULL; >>> if (!component) >>> return av_strdup(path); >>> >> >> isn't this what av_strdup already does? > > It's not documented to do so. It could also decide to treat > av_strdup(NULL) as av_strdup(""). Even if undocumented, that would be an unexpected change in behavior. I'm with Vittorio, we should mention the actual behavior in the doxy and make it official.
James Almer: > On 3/25/2024 1:55 PM, Andreas Rheinhardt wrote: >> Vittorio Giovara: >>> On Mon, Mar 25, 2024 at 12:38 PM Andreas Rheinhardt < >>> andreas.rheinhardt@outlook.com> wrote: >>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavutil/avstring.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavutil/avstring.c b/libavutil/avstring.c >>>> index 2071dd36a5..8702fe0455 100644 >>>> --- a/libavutil/avstring.c >>>> +++ b/libavutil/avstring.c >>>> @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path, >>>> const >>>> char *component) >>>> char *fullpath; >>>> >>>> if (!path) >>>> - return av_strdup(component); >>>> + return component ? av_strdup(component) : NULL; >>>> if (!component) >>>> return av_strdup(path); >>>> >>> >>> isn't this what av_strdup already does? >> >> It's not documented to do so. It could also decide to treat >> av_strdup(NULL) as av_strdup(""). > > Even if undocumented, that would be an unexpected change in behavior. > I'm with Vittorio, we should mention the actual behavior in the doxy and > make it official. The problematic thing is that this behaviour is also mostly insane. Or rather: If you care to distinguish "returns NULL due to allocation failure" and "returns NULL due to NULL src", then you have to check src for NULL anyway and then you could also just have checked before calling av_strdup and avoided it in case src is NULL. This code here is the only example I found for which the current behaviour is actually reasonable. - Andreas
diff --git a/libavutil/avstring.c b/libavutil/avstring.c index 2071dd36a5..8702fe0455 100644 --- a/libavutil/avstring.c +++ b/libavutil/avstring.c @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path, const char *component) char *fullpath; if (!path) - return av_strdup(component); + return component ? av_strdup(component) : NULL; if (!component) return av_strdup(path);
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavutil/avstring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)