Re: Clean up OSL_ASSERT, DBG_ASSERT, etc.

On 01/16/2015 01:26 PM, Ashod Nakashian wrote:
[...]
> There are numerous different assertion functions/macros used in the
> Libreoffice codebase, each with its own behavior and implementation.
> This is problematic in its own right, as there is no obvious way to
> know what to expect when a certain precondition is broken (by new

What do you mean with "what to expect" here?

> code, modified code, or by a new external input,) which is vital for

Not sure I understand you here, but note that input data in itself 
should never be the reason for a failed precondition.  A failed 
precondition is always an indicator of a programming error.

[...]
> A quick grep returns the following assertion functions (those
> containing case-insensitive "assert"): assert, DBG_ASSERT, OSL_ASSERT,
> DBG_BF_ASSERT. This is excluding CPPUNIT_ASSERT and assertions in
> 3rdparty libraries (which are out of scope).

History shows that moving from the legacy zoo of ill-specified, 
inconsistently-used functionality to a supposedly sane set is by no 
means trivial and is taking a long long time to accomplish.

It is the inconsistent usage of the legacy functionality that makes it 
hard to properly map to one of the three to four advocated mechanisms du 
jour (static_assert, assert, SAL_LOG_IF, SAL_WARN_IF), and that 
apparently makes easy hackers shy away from it.

> I know there has been a push to use the system assert, which at least
> on Windows aborts, which has the nice side-effect that it can't be

It aborts on all platforms (unless disabled with NDEBUG, of course).

[...]
> 1) Static assertion: Compile-time assertion that fails the build if
> violated, otherwise compiles into nop in all configurations.
> LO_ASSERT_STATIC(EXP);

I see no real benefit in wrapping a macro around C++11 static_assert.

> 2) Permanent assertion: Runtime assertion that is never compiled out
> of the code, even in releases (see below). These are very cheap, but
> critical, checks. Typically trivial bounds or invariant checks. The
> overhead of such an assertion should be <5% (typically 2%) of the
> function they are in.
> LO_ASSERT_REL(EXP);
>
> 3) Debug assertion: Runtime assertion that validates pre- and
> post-conditions, including some checks that require some nominal
> amount of computation. These are the typical debug assertions and
> should have an overhead of 5-20%. Enabled for non-Release builds only.
> LO_ASSERT(EXP);
>
> 4) Sanity/Validation assertion: Extensive runtime assertions that
> validate an algorithm by executing extensive checks and
> recomputations. These checks have a too high a cost to be enabled in a
> build used by even the wider development community. They are best
> enabled when debugging a certain algorithm or tracking down a
> particularly nasty issue. The overhead of these assertions are >20%.
> Enabled only when LO_USE_ASSERT_CHECKS is defined.
> LO_ASSERT_DEBUG(EXP);

I am not sure we would manage to maintain a useful discrimination of 
assert-uses based on their (perceived) runtime impact.

(We have OSL_DEBUG_LEVEL to disable---and probably let bit-rot---the 
occasional assertion code that would be prohibitively expensive in 
general, and that appears to work, kind of.)

[...]
> 1) FailLog: Prints to stderr and a log file (optional). (Ideally will
> include the stack-trace as well.)

What is the benefit of not aborting the out-of-control process immediately?

[...]
> 3) FailBreak: Calls FailLog then breaks into the debugger (typically
> by issuing 'int 3' on x86/x64). (This might not be useful on
> non-Windows platforms.)

My understanding is that Windows alrady offers this "break into the 
debugger" for plain abort?

> 5) FailThrow: Calls FailLog then throws std::logic_error. (This
> assumes there are handlers, or a debugger is set to break on C++
> exceptions.)

std::logic_error implying a programming error, I see no better approach 
anyway for LO than to abort in the event of a thrown logic_error.

> Once the support code is created, this is solidly an easy hack, one
> that beginners should be encouraged to work on.

see above