On Wed, Jul 25, 2018 at 1:53 AM, Wei Liu <wei.l...@citrix.com> wrote:
> On Wed, Jul 25, 2018 at 09:49:39AM +0100, Wei Liu wrote:
> > On Sat, Jul 21, 2018 at 02:14:12AM +0200, Marek Marczykowski-Górecki
> wrote:
> >
> > > +
> > > + memcpy(dest, buf, len);
> > > + for (i = 0; i < len; i++) {
> > > + if (dest[i] == '\033')
> > > + dest[i] = '.';
> > > + }
> >
> > This could be made more efficient by using:
> >
> > for (i = 0; i < len; i++) {
> > if (src[i] == '\033')
> > dst[i] = '.';
> > else
> > dst[i] = src[i];
>
The above code doesn't write the value that was checked into the
destination buffer; instead it does a second copy from the source buffer.
That is a problematic code pattern that we shouldn't really encourage.
>
> Oh this can even be written in a shorter form:
>
> dst[i] = src[i] != '\033' ?: '.';
>
>
That may be shorter but it is harder to understand, whereas Marek's
original code is clear and well structured.
One nit with the proposed patch is the introduction of the dynamically size
array on the stack, with input to the function determining its size:
> static int write_all(int fd, const char* buf, size_t len)
> {
> + char buf_replaced[len];
That might be just fine for userspace code, but would it be better replaced
with a fixed size buffer and a loop instead?
Christopher
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
