On Mon, Feb 06, 2017 at 02:04:52PM -0800, Evan Gates wrote:
> Hiltjo Posthuma <hiltjo_AT_codemadness.org> wrote:
>
> > Probably a false positive, but for ed.c it looks ok to me.
> >
> > The join.c patch does not look ok to me (clang doesn't detect that
> > eprintf() exists the program), so it's a false positive.
>
> This is where the _Noreturn or noreturn attribute is handy.
> Unfortunately that's C11.
>
> It seems ed.c is the same problem, error() has a longjmp() instead of
> return causing a false positive.
>
> I do like getting rid of warnings but I'm not particularly fond of
> changes purely for that reason.
>
> As for join.c, if we do care about getting rid of warnings we can
> rewrite to take advantage of the initialized values instead of assigning
> again when s is "0". Although I'm not sure the intent is as clear this
> way. Example patch below.
>
> I'd like to hear more thoughts on this, if the community's response is
> overwhelmingly one way or the other I'll gladly accept the changes.
>
> ----- 8< ----- 8< -----
>
> From 5eae9c0bd10c6da57fcd8527e7bbc6c23cda4b08 Mon Sep 17 00:00:00 2001
> From: Evan Gates <evan.gates_AT_gmail.com>
> Date: Mon, 6 Feb 2017 13:50:23 -0800
> Subject: [PATCH] Initialize to avoid false positive clang warning
>
> clang doesn't recognize eprintf exits and warns about possibily
> uninitialized values. Initialize the values and rearrange the
> conditionals to avoid redundant assignment.
> ---
> join.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/join.c b/join.c
> index d3e2343..e62bb8f 100644
> --- a/join.c
> +++ b/join.c
> _AT_@ -314,16 +314,13 @@ static struct spec *
> makespec(char *s)
> {
> struct spec *sp;
> - int fileno;
> - size_t fldno;
> + int fileno = 0; /* default to "0", join field must be 0 and nothing else */
> + size_t fldno = 0;
>
> - if (!strcmp(s, "0")) { /* join field must be 0 and nothing else */
> - fileno = 0;
> - fldno = 0;
> - } else if ((s[0] == '1' || s[0] == '2') && s[1] == '.') {
> + if ((s[0] == '1' || s[0] == '2') && s[1] == '.') {
> fileno = s[0] - '0';
> fldno = estrtonum(&s[2], 1, MIN(LLONG_MAX, SIZE_MAX)) - 1;
> - } else {
> + } else if (strcmp(s, "0")){
> eprintf("%s: invalid format\n", s);
> }
>
> --
> 2.11.0
>
>
Looks ok to me.
--
Kind regards,
Hiltjo
Received on Tue Feb 07 2017 - 09:51:38 CET