[boost] Re: operators.hpp bugs

From: Søren Lassen (s.lassen_at_[hidden])
Date: 2005-04-28 09:57:23

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. In the context, it can be read as either a T (or  
something implicitly convertible, e.g. "&T"), or as something that can be  
used in an assignment/construction (e.g. also "const T", "const &T"). The  
required non-const-ness of the return values is not obvious, and no  
rationale is given for it in the documentation, nor in the program  
comments.

Quoting from http://www.boost.org/more/lib_guide.htm#Rationale_rationale:
"Beman Dawes comments:  Failure to supply contemporaneous rationale for  
design decisions is a major defect in many software projects. Lack of  
accurate rationale causes issues to be revisited endlessly, causes  
maintenance bugs when a maintainer changes something without realizing it  
was done a certain way for some purpose, and shortens the useful lifetime  
of software."

>
>> 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"? 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?

>
>> 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). 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. 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.

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.

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.

-- 
Søren