Hi,
Eike Rathke wrote:
Hi Stephan,
On Wed, Aug 31, 2005 at 10:04:47 +0200, Stephan Bergmann wrote:
Sorry for the (may be stupid) question, but why not just change
OSL_VERIFY to emit nothing, in case OSL_DEBUG_LEVEL == 0? I would expect
that only weird code would relay on the evaluation in case of a zero
debug level. And these case can probably easily be changed to something
with "assert" (or ensure).
No, the code I saw was of the form
OSL_VERIFY(close(f) >= 0);
Well, to me that _is_ weird code. The programmer tried to save
a variable and an assignment of the return value to the variable, but
trades that in for not detecting an unsuccessfull close() in an
OSL_DEBUG_LEVEL == 0 build, and not reacting on it. That's not only bad
style, that's ugly, and maybe even wrong code.
If the file that is being closed was written to, this is almost
certainly a bug.
If the file was opened read-only, then ignoring the result of close() is
generally OK. In that case the programmer may have added this assertion
to catch cases where the close fails due to an invalid file descriptor
(e.g. a double close), which would indicate a logic error, or simply to
document their (erroneous) assumption that close() can't fail after
reading a file. So for a read-only file this might be acceptable, but as
it is of very limited utility even in that case, the assertion should
better be dropped.
Leaving statements of debug macros in a product version most times leads
to unnecessary code being executed, instead nasty uses such as above
should be detected and eliminated and then OSL_VERIFY() set to empty for
OSL_DEBUG_LEVEL == 0.
As stated elsewhere: It is the one and only purpose of OSL_VERIFY to
provide a construct that allows asserting on a expression that must be
evaluated every time, including in the product version. All uses of that
macro should contain expressions with a side effect.
Your proposal is to make OSL_VERIFY identical to OSL_ASSERT (in which
case it should be dropped completely). The nastyness in the above
example is not that code with side effects is embedded in a debug macro
(that is as intended). The nastiness is that an assertion is used in
place of proper handling of runtime errors. But that is not specific to
OSL_VERIFY. There are loads of places in the office where error return
codes or caught exceptions are 'reported' by an OSL_ASSERT or OSL_ENSURE
- and then ignored.
BTW: In my experience the usual debug macros have the potential for much
more serious errors, when code with side effects is used in an
assertion. In that case they may cause necessary code to not be executed
in product versions or they may change the entire flow of control,
leading to hard to debug problems.
Btw, since when would a close() return a value > 0 ?!?
Apparently this is inverting the common logic of checking
if (close(f) < 0) handle_error();
But I agree that it looks unusual and ==0 should be used to check success.
Ciao, Jörg
--
Joerg Barfurth Sun Microsystems - Desktop - Hamburg
>>>>>>>>>>>>>>>>>> using std::disclaimer <<<<<<<<<<<<<<<<<<<<<<<
Software Engineer [EMAIL PROTECTED]
OpenOffice.org Configuration http://util.openoffice.org
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
