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? _______________________________________________ Wireshark-dev mailing list [email protected] http://www.wireshark.org/mailman/listinfo/wireshark-dev
