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 <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.
>
Received on Tue Feb 14 2017 - 15:00:33 CET