On 2020-07-19 23:38 +0200, Andreas Rheinhardt wrote:
> Alexander Strasser:
[...]
> >
> > On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
> >> Andreas Rheinhardt:
> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com>
> >>> ---
> >>> libavformat/au.c | 13 ++++++-------
> >>> 1 file changed, 6 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/libavformat/au.c b/libavformat/au.c
> >>> index f92863e400..b419c9ed95 100644
> >>> --- a/libavformat/au.c
> >>> +++ b/libavformat/au.c
> >>> @@ -68,13 +68,13 @@ static int au_probe(const AVProbeData *p)
> >>>
> >>> static int au_read_annotation(AVFormatContext *s, int size)
> >>> {
> >>> - static const char * keys[] = {
> >>> + static const char keys[][7] = {
> >>> "title",
> >>> "artist",
> >>> "album",
> >>> "track",
> >>> "genre",
> >>> - NULL };
> >>> + };
> >
> > Sorry, I couldn't test myself but wouldn't just
> >
> > static const char * keys[] = {
> > "title",
> > "artist",
> > "album",
> > "track",
> > "genre",
> > };
> >
> > have been the better choice with the same benefits?
> >
> > I'm not sure without looking up the C standard and writing some
> > little test programs. From my guts I would say it's equivalent,
> > but the syntax is more convenient this way.
> >
> > That's also another issue with the commit message, since I don't
> > think it is true that in your version strings are stored in the
> > array. No offense intended and I sure might be mistaken.
> >
> But it is true.
Yes, you're right. I read the array dimension the wrong way around :(
Sorry for the noise.
> The strings are stored directly in the array, so that
> each array takes up 5 * 7 bytes. Before the change, the array itself
> took up 5 * sizeof(char *). And on top of that comes the space for the
> strings itself.
>
> Storing the strings itself in the array instead of pointers to the
> strings saves the space of the pointers, but it forces the shortest
> string to the size of the longest string. Therefore it is worthwhile if
> the average size differs from the longest size by less than sizeof(char
> *); there is furthermore one exception to this rule: If one stores
> pointers, different pointers may point to the same string (or to a part
> of a larger string). In the case here, the strings itself are smaller
> than sizeof(char *) on 64bit systems, so this alone shows that one saves
> space.
So it's about 40 bytes on systems with 64 addresses, right?
> Also notice that there are two more differences:
> a) The earlier version consisted of an array of nonconst pointers to
> const strings. Making the array entries const has been forgotten (it
> would go like this: "static const char *const keys[]"). Given that
> arrays are automatically nonmodifiable, this problem doesn't exist.
> (Given that these arrays are static and given that the compiler can see
> that they are not modified means that the compiler could infer that they
> are const and put them in the appropriate section for const data.)
> b) The actual access to the string is different: If the array contains
> pointer to strings, then one has to read the array entry (containing the
> pointer to the string) and pass it to av_strcasecmp() etc. If the array
> contains the strings, then one just has to get the address of the array
> entry (which coincides with the address of the string itself) and pass
> it to av_strcasecmp() etc..
I agree the new version is technically superior.
It's also more rigid, but that should probably not matter much in
this specific case.
Still I think the savings are marginal, so I guess you did them
while looking into the code for fixing the things later in this
patch set.
Fair enough.
A small additional sentence in the commit message body, would have
been better IMHO:
Save a bit of space and one layer of indirection.
Best regards,
Alexander
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
