Re: [boost] BOOST_ASSERT and __assume

From: Dave Harris (brangdon_at_[hidden])
Date: 2005-09-24 05:26:41

In-Reply-To: <dh1gjf$bt0$1_at_[hidden]>
kalita_at_[hidden] (Marcin Kalicinski) wrote (abridged):
> On the other hand some people (including me) consider __assume to be 
> nice, helping optimization and suppressing annoying warnings.

Including me, too. I don't think anyone has spoken against __assume in 
principle. The issue has been how best to incorporate it.

> Put __assume in BOOST_ASSUME and leave BOOST_ASSERT unmodified.

This to me is the safest option.

> Pros:
> - does not disturb existing code in any way
> 
> Cons:
> - no existing code uses it, i.e. all existing libraries would have to 
> be modified to take advantage of it; this will probably never happen so 
> most of the code will not benefit

__assume is a performance optimisation. As such I think it is fine for it 
to be adopted primarily in places where someone has seen the need (and in 
new code). It doesn't need to be adopted in /all/ libraries.

If no-one thinks __assume is valuable enough to be worth changing old code 
to use it, perhaps it is not worth adding to boost at all. Is it entirely 
a Microsoft thing, or would some other compilers benefit too?

> - obscure: most of the people would be confused about it when seeing it 
> for the first time

Surely no more so than any other boost library? I'd have thought it would 
be even more confusing if BOOST_ASSERT changed its meaning. At least the 
people who don't know what ASSUME means know they don't know, and can look 
it up. A change to ASSERT will slip under many people's guard.

BOOST_ASSERT and BOOST_ASSUME have different semantics. I think it's good 
to let authors express which semantics they intended.

> 2. BOOST_ASSERT + define to enable assume (default: on)
> Cons:
>
> - breaks user code which uses BOOST_ASSERT in "uncommon" way.

And boost code as well. Probably there is no such code in boost, but I 
don't think anyone has gone through it all to check. Perhaps no user code 
would be broken either.

To me this really feels like a gambler's option. Silently breaking users 
code is unacceptable, but maybe we are feeling lucky tonight. Or maybe we 
just don't care, and can rule such code unsupported.

> BOOST_ASSERT + define to enable assume (default: off)
> - does enable assume on all the boost code, some user programs get 
> smaller and faster with a simple -D"BOOST_ASSERT_USE_ASSUME" on command 
> line.

In order to switch it on safely they need to know that every use of 
BOOST_ASSERT in every module they link, and every header they #include, is 
safe with the new semantics. I don't see how they can know this without 
laboriously checking every place where BOOST_ASSERT is used and thinking 
hard about the semantics of each one. This has to be repeated for every 
new release of the libraries.

It's a ton of work, repeated once for each user. To me it makes far more 
sense for the work to be done once by the author of the code that uses the 
assert, and for the result of that analysis to be recorded in the code for 
all time by renaming from BOOST_ASSERT to BOOST_ASSUME. This will also 
help anyone reading the code.

Another drawback of the global flag is that if just one place turns out to 
require the old semantics, no other places can use the new semantics. The 
whole app suffers. (It'd be tempted to define BOOST_ASSERT_USE_ASSUME for 
some modules and not others, but that leads to violation of the One 
Definition Rule.)

In practice I think this option is the same as the previous one. Both are 
saying the "uncommon" uses are not supported any more. Certainly they 
can't be used within any library code, because the author doesn't know 
what options the user will compile it with. I don't think we want to 
double the testing burden by testing both with and without 
-D"BOOST_ASSERT_USE_ASSUME".

The effect of -D"BOOST_ASSERT_USE_ASSUME" is to inject undefined behaviour 
into the code. To me that is huge. It's not a minor change of semantics.

-- Dave Harris, Nottingham, UK.