[boost] Re: operators.hpp bugs

From: David Abrahams (dave_at_[hidden])
Date: 2005-04-28 11:16:31

Søren Lassen <s.lassen_at_[hidden]> writes:

> On Wed, 27 Apr 2005 08:01:36 -0400, David Abrahams  
> <dave_at_[hidden]> wrote:
>
>> Søren Lassen <s.lassen_at_[hidden]> writes:
>>
>>> On Tue, 26 Apr 2005 12:44:19 -0400, David Abrahams
>>> <dave_at_[hidden]> wrote:
>>>
>>>>
>>>> It was discussed in the past IIRC and Philippe's rationale basically
>>>> captures the consensus here.
>>>>
>>>
>>> Very well, it seems that I am outnumbered. Non-const return values from
>>> binary operators are not a bug, but a feature. However, it remains an
>>> undocumented feature.
>>
>> ??  The return type of the generated operators is perfectly well
>> visible in the documentation.
> The documentation (http://www.boost.org/libs/utility/operators.htm) says  
> that the operators return "convertible to T". In the broadest sense, this  
> can mean anything. 

No, you have to read the table's header.  That's a **Requirement** on
operations (such as operator+=) that are used by the generated
operators (such as operator+).  The exact return types of the
generated operators are spelled out precisely in the table's
**Supplied Operations** column.

>>> It would be easier to contribute to boost if the programming
>>> guidelines on the homepage
>>
>> What are you referring to?  There are no programming guidelines in
>> http://www.boost.org/index.htm.
>
> There is a link on the left (section "About") that says "Guidelines"
> it takes you to http://www.boost.org/more/lib_guide.htm#Guidelines
> The section contains, among other things, programming
> guidelines. They are, in some cases, quite detailed.  But you are
> right. The Guidelines are not on the actual home page.
>>
>>> corresponded to the actual programming guidelines that have been
>>> decided.
>>
>>> At the moment the homepage has an explicit reference to a book
>>> ("Effective C++", second edition) that recommends the opposite
>>> policy, the real guidelines are not mentioned.
>>
>> There are no "real guidelines."  Decisions are taken on a case-by-case
>> basis.
>
> Then why does the homepage have a link called "Guidelines"? 

There are guidelines for library developers.  One is "consult
Effective C++."  There are no "real programming guidelines."

> And why do you refer to a past discussion (what is IIRC, by the way?
> there are no references on the www.boost.org website), if decisions
> are taken on a case-by-case basis?

Umm, because the past discussion is what led to the decisions in this
case?

>>> I have reintroduced the non-const return values for the operators,
>>> the new version has been uploaded to the sandbox vault.
>>
>> What is this code supposed to fix?  The first things you should
>> submit are test programs that fail to work because of the things
>> you are saying are broken.
>>
> I tried to fix the following errors:
>
> 1) An apparently worthless speed optimization for NRVO, which, for
> compilers that only have RVO, is probably worse than no optimization
                                   ^^^^^^^^
> (because the call-by-value method introduces a named value that is
> harder to optimize away). 

The NRVO and RVO stuff was extensively analyzed by the people that
contributed it years ago.  There are config macros to detect when RVO
and NRVO are available.  Why should we believe your estimate of what
"is probably worse" over the work that was done earlier?

> My fix also makes the program easier to read and maintain.
>
> I do not have examples of code that fails, it does not (were there
> any examples of failing programs that were fixed by the NRVO
> optimization? I think not). The biggest problem is the unnecessary
> complicatedness of the program, and the fact that "operators.hpp",
> when used as an example, may entice other programmers to write
> similarly over-convoluted code. 

Our users aren't supposed to look at our implementations to figure out
what to do.

> If you really want Boost to be used
> widely and to be seen as an example of good programming, "if it
> ain't broke, don't fix it" is not the right attitude, continous
> improvement is.

The code didn't start out that complicated.  It became that
complicated for a reason.

> By the way, the documentation of the NRVO code is very good, the rationale  
> is well-explained and well-documented. That is exactly why I am pretty  
> sure that
>   a) the rationale unfortunately is a misunderstanding,
>   b) the optimization can safely be removed.

This just makes no sense.  The documentation is well-explained,
therefore it's completely mistaken?  So far, you're making no sense to
me.  I suggest you take this up with the library's current maintainer,
Daniel Frey, who is also responsible for all the (N)RVO stuff
(http://lists.boost.org/MailArchives/boost/msg44891.php).

> 2) A pollution of the global namespace, which creates a potential  
> ambiguity. The following examples will fail with compilers that do not  
> support inline friend functions in namespaces:
>
> 	#include <boost/operators.hpp>
> 	using namespace boost;
> 	class a:addable1<a>{...}
> 	// Error: ambiguity between boost::addable1 and ::addable1
>
> 	#include <boost/rational.hpp> // includes operators.hpp
> 	bool addable1;
> 	// Error: addable1 is already defined in global namespace
>
> The problem is not only that the above code fails on these (this?)
> platforms, but also that it succeeds on other platforms. Thus,
> programmers very easily end up writing non-portable code that has to
> be rewritten at a later stage.  My fix for this bug also makes the
> code easier to read and maintain.

Did you submit a test that fails because of this bug?  As I've been
trying to tell you, that's the first step.

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com