Julian Foad wrote on Mon, Apr 23, 2012 at 21:11:30 +0100: > > > > > ----- Original Message ----- > > From: Daniel Shahaf <danie...@elego.de> > > To: Julian Foad <julianf...@btopenworld.com> > > Cc: Bert Huijben <b...@qqmail.nl>; "dev@subversion.apache.org" > > <dev@subversion.apache.org> > > Sent: Monday, 23 April 2012, 20:59 > > Subject: Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in > > /subversion/trunk: ./ subversion/libsvn_delta/compat.c] > > > > Julian Foad wrote on Mon, Apr 23, 2012 at 20:40:59 +0100: > >> Daniel Shahaf wrote: > >> > >> > Julian Foad wrote: > >> >> I (Julian Foad) wrote: > >> >> > There isn't currently an easy build switch (such as > > NDEBUG) to disable > >> >> > SVN_ERR_ASSERT completely at compile time. That's just > > a side issue. If > >> >> > you want such a switch, just ask; we can easily create one. > > Or if you think we > >> >> > need two levels of assertions -- one for quick tests and > > another for slow tests > >> >> > -- and want to be able to compile-out the slow ones > > independently of the quick > >> >> > ones, just ask. But implying we should use 'assert' > > for slow tests and > >> >> > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way. > >> >> > >> >> We can also introduce run-time control of whether the conditions > > are > >> >> evaluated: test a global 'assertions enabled?' variable or > > function > >> >> before evaluating the condition. For example: > >> [...] > >> > That doesn't sound right. Surely we don't want to allow > > disabling _all_ > >> > uses of SVN_ERR_ASSERT() this way? (Remember that some of them > >> > translate to segfaults (possibly corruptions?) if the condition > > doesn't > >> > hold) > >> > >> Hi Daniel. > >> > >> In places where there will be a seg-fault if the condition is false, > >> the assertion statement doesn't prevent abnormal program termination, > >> it only makes it easier to see what went wrong. > >> > > > > SVN_ERR_ASSERT() prevents an abnormal termination when a non-default > > malfunction handler has been installed. > > Potentially, but the app can't tell how badly the library data has > gone wrong, so may want to terminate the program anyway after > informing the user. (Quoting an email (from Steve King?) within the > past year or two.) >
Yes. We don't distinguish various kinds of asserts, so the app needs to assume the worst. But it's a cleaner termination than SIGABRT. > >> In places where the processing will continue with wrong data or wrong > >> behaviour if the condition is false, the assertion statement doesn't > >> prevent the program from going wrong, it just changes the failure mode > >> to a more obvious one. > >> > >> People who don't care about the failure mode in such cases may wish to > >> turn off the checks. > > > > Depends, I think. If libsvn_ra asserts that a log message is in UTF-8, > > it is reasonable to want to disable that since the only harm resulting > > is an svn_error_create() on the server. > > How do you know it won't crash? > The requirement for UTF-8 is at the repos layer, so at least ra_local and ra_svn should cope just fine with arbitrary binary data in there. I don't recall whether that's true for ra_dav too. But I think that whether or not libsvn_ra* segfault on non-UTF-8 log message wasn't your point. > > But if libsvn_fs asserts the > > sanity of a revision before committing it, I think most people will > > prefer to have the commit attempt fail. > > > > (The latter example is not realistic since, at least in FSFS, we > > generally return an svn_error_t rather than an assertion when a sanity > > check fails.) > > - Julian
