Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 21865: /trunk/epan/ /trunk/epan/: fo
Joerg Mayer wrote:
On Mon, May 21, 2007 at 10:03:08PM +0200, Ulf Lamping wrote:
put fwrite and fread into DISSECTOR_ASSERT in order to use the result
Is it a good idea to put "productive" code into an DISSECTOR_ASSERT?
At least the ANSI C assert() won't call any comparison code at all in a
release version.
Well, as we call an IO function, the overhead of the comparison should be
negligible.
I didn't meant the overhead of the comparison.
If you would do something like:
assert(1 * sizeof(tcp_stream_chunk) == fwrite( sc, 1,
sizeof(tcp_stream_chunk), data_out_file ));
in production code, the fwrite function would *never be called* (at
least if the assert is used as intended by ANSI C).
So having productive code between the assert brackets is a very bad idea
for normal assert(), and should be avoided IMHO for functions named
XY_ASSERT as well.
If we agree that productive code can be used inside a DISSECTOR_ASSERT,
we should give it a different name than DISSECTOR_ASSERT :-)
However, at least it's a bit better than before ;-)
Well, it gets rid of a warning on Suse systems which prevented
compilation, but whether it is an improvement: I don't know....
But that kind of stuff happens if you only have limited time (and
knowledge of the code involved) but *must* fix a warning due to
-Werror.
Yes, I know from my own experience, that fixing warnings of code not
written by oneself can be pretty annoying. That was one of the reasons
that I started the "threat warnings as errors" campaign, so you'll at
least notice such bugs much earlier (at least if you don't ignore the
buildbot).
And not checking the return value of fwrite() is simply a bug IMHO. If
fwrite() fails for whatever reasons, the user didn't get any response
(except for a crippled output file). However, with using
DISSECTOR_ASSERT(), the user will get tons of console outputs (I guess
one for each packet) - which might be pretty annoying ...
Fixing warnings by "short term" solutions won't make our code much
better - and get us into even more trouble in the long term (as it might
contain design bugs, which are pretty hard to fix later, if a lot of
code is based on this). Any ideas how to avoid such things creeping in,
for the future?
Regards, ULFL
P.S: I'm confused, why the different platforms using gcc showing
different behaviour. Both the Ubuntu and Solaris buildbots also using
gcc and don't show the problem?!? Or are you using different compiler
options for SuSE?
