do..while(0) idiom causes compiler warnings

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Compile the code with either Microsoft or Solaris compiler
2.
3.

What is the expected output? What do you see instead?

Expect clean compiling code. Get warnings.

What version of the product are you using? On what operating system?

0.3.1

Please provide any additional information below.

Definitions of PB_RETURN_ERROR use the do..while(0) idiom.  Unfortunately lots 
of compilers object to this:

-  My Windows compiler goes:

pb_encode.c(92) : warning C4127: conditional expression is constant

-  My Solaris compiler goes:

pb_encode.erl "pb_encode.c", line 97: warning: end-of-loop code not reached

I propose simply not doing the do..while(0) thing and instead just define the 
macro with its own scope.  So far as I can see, all that this costs us is that 
if you forget to type a semi-colon after calling PB_RETURN_ERROR() you won't 
get a compiler error - but nothing will go wrong either, so this isn't the end 
of the world.

Proposed patch is attached.

Thanks!

David

Original issue reported on code.google.com by david.ho...@googlemail.com on 2 Jan 2015 at 11:25

Attachments:

• 0001-Fix-compiler-warning-on-do.while-0.patch

[GoogleCodeExporter]

Alas, I was too hasty with the previous fix - the picky Solaris compiler still 
objects.  This time is spews out warnings like this:

CC problem pb_decode.erl "pb_decode.c", line 272: warning: statement not reached

The problem being that after macro expansion the compiler sees the empty 
statement caused by the final semi-colon and, as above, complains that this 
statement is not reached.

Attached a patch that really does fix this, by retiring PB_RETURN_ERROR() and 
replacing it with PB_SET_ERROR(), requiring the caller to "manually" return 
false himself.

This is a rather more intrusive fix than I'd have liked and - while I do think 
that the code is slightly cleaner as a result - I'll quite understand if you 
prefer to say "no this is too much, what you should do instead is just turn off 
these compiler warnings".

What do you think?

Original comment by david.ho...@googlemail.com on 2 Jan 2015 at 12:52

Attachments:

• 0001-Larger-fix-for-do.while-0-warnings.patch

[GoogleCodeExporter]

The PB_SET_ERROR() way is not acceptable, it is too easy to forget one of the 
lines. Error handling is quite laborious to check, so I prefer to make it very 
hard to mess up.

The {} alternative also breaks in this case:
if (foo)
PB_RETURN_ERROR(...);
else
...
but that atleast gives a compiler error and is easy to fix by removing the ;.
Too bad it didn't help with the solaris compiler.

Something like this is also an alternative, though less clear to the reader:
return ((stream)->errmsg == NULL ? ((stream)->errmsg = (msg)) : 0), false

Does that warn on the compilers you are interested in?

Original comment by Petteri.Aimonen on 2 Jan 2015 at 7:52

• Added labels: Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Aha, clever.  Yes, that works for me.

Perhaps it would be fractionally more readable to define my PB_SET_ERROR() 
anyway - it makes a natural partner to PB_GET_ERROR() - and then 

#define PB_RETURN_ERROR(stream,msg) \
        return PB_SET_ERROR(stream,msg), false

All variations along these lines seem to work out just fine on all of the 
compilers that I care about so I'd be very happy for you to pick whatever seems 
least ugly to you.

Thanks!

David

Original comment by david.ho...@googlemail.com on 2 Jan 2015 at 11:54

[GoogleCodeExporter]

This issue was updated by revision b0d31468da7f.

Original comment by Petteri.Aimonen on 3 Jan 2015 at 9:00

• Changed state: FixedInGit

[GoogleCodeExporter]

Fix released in nanopb-0.3.2.

Original comment by Petteri.Aimonen on 24 Jan 2015 at 3:53

• Changed state: FixReleased