This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Fri, 1 Apr 2016 14:55:49 -0700 (PDT)
- Subject: Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]
- Authentication-results: sourceware.org; auth=none
- References: <56E339A7 dot 7060704 at redhat dot com> <20160311222757 dot DB90C2C3C24 at topped-with-meat dot com> <56FBBA94 dot 1040605 at redhat dot com> <20160330232737 dot 2A3F32C3C35 at topped-with-meat dot com> <56FD5B69 dot 1010002 at redhat dot com>
> I can remove the use of d_namlen as a separate patch, before the copy
> avoidance. I did not realize it has implications on application
> behavior at the time, for which I apologize. But now, I still think
> it's a desirable improvement.
That sounds great.
> Sorry, I don't follow. I expressed a preference for a coding strategy
> that reduces risk by aligning behavior across architectures, so that we
> can reuse the results of testing on one set of architectures for others.
My point was that you included a change that did not seem relevant to the
stated goal, and your response to my review still didn't clearly say why
the status quo had to be changed. Separating small changes out into their
own review threads is the best way to be sure that the rationale for the
change was stated clearly and gets discussed fully.
> > While inlines are better than macros when all else is equal, this
> > duplication of something that lends itself to a single-expression form
> > (?:) is a good reason to make it a macro instead.
>
> The call site relies on the function nature because the source argument
> is evaluated multiple times. I could change that by using a temporary
> at the call site, but ...
>
> > Also, compound literals are a GNU extension and I'm not sure if they
> > meet gnulib's portability requirements or not.
>
> ... avoiding the compound literal requires writing a function. I could
> put the shared body into a macro, though.
As you can tell, I didn't drill down to all the details of what's possible.
I just noticed the glaring duplication and wanted to push back on that.
Sometimes duplication is the best of the available options, which are all
imperfect. When that's so, it merits a comment about why duplication is
what makes the most sense. The way to read my comment is, "This looks like
a fishy level of duplication; see if you can find something better, or
explain why this is the best we can do."
Thanks,
Roland