Perhaps following the eprintf() with return NULL;
would inform the compiler correctly without changing the semantics of the program. I worry if a smarter compiler would just warn about the statement being unreachable though. Warning cat-and-mouse is probably something to avoid. - Simon On 07/02/17 08:51, Hiltjo Posthuma wrote: > On Mon, Feb 06, 2017 at 02:04:52PM -0800, Evan Gates wrote: >> Hiltjo Posthuma <[email protected]> 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 <[email protected]> >> 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 >> @@ -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. >
signature.asc
Description: OpenPGP digital signature
