bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Mon, 22 Jan 2018 22:25:39 +0000
> Cc: 30005@debbugs.gnu.org
> 
>  Does the patch below look right, and give good results?
> 
> Yes, thanks. Just some minor nits inline to make the code shorter.

Thanks for the review.  I eventually pushed the changes as presented
here, for the reasons I explain below.

>     Lisp_Object prefix_arg;
>  -  char *string;
>  +  char *string, *string_end;
>  +  ptrdiff_t string_len;
> 
> I think these days (where we require C99) we always declare variables when we 
> first use them.

That's not my understanding of the preferred style.  My understanding,
and what I do in practice, is that variables used only in a small
portion of a function should be declared before the use, so that all
the references to those variables are localized to the fragment where
they are used.  By contrast, in this case these variables are used all
over the function, so it makes much less sense to delay their
declaration.

>  -      tem = strchr (tem, '\n');
>  +      tem = memchr (tem, '\n', string_len - (tem - string));
> 
> You can write the third argument as string_end - tem.

Yes, but IMO the above is easier to convince the reader that the code
is correct, and an optimizing compiler will produce the same code from
both.

>  -      visargs[1] = make_string (tem + 1, strcspn (tem + 1, "\n"));
>  +      char *pnl = memchr (tem + 1, '\n', string_len - (tem + 1 - string));
> 
> Here you can write the third argument as string_end - (tem + 1).

Same here: I find the code I used easier to understand and verify its
correctness.

>  +      ptrdiff_t sz = pnl ? pnl - (tem + 1) : string_end - (tem + 1);
> 
> You can write the RHS as (pnl ? pnl : string_end) - (tem + 1).

Same here.

Thanks.