[boost] Re: operators.hpp bugs

From: Daniel Frey (d.frey_at_[hidden])
Date: 2005-04-28 12:06:33

Sorry for my late reply, but my PSU failed a few days ago and I had to 
get a new one *twice* :(( Now I'm back online.

Søren Lassen wrote:
> 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.

If you think that this is an undocumented feature, than I can show you 
hundreds of other undocumented features. You can't document everything. 
Also, it would be nice if you don't start by telling us you found a bug 
but to ask about whether or not this is an oversight or if this has been 
discussed before or something. Mostly because it turns out that we 
discussed almost everything already :)

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

There is no "required non-const-ness". How do you read that from the 
documentation? As a matter of fact, there is no required const-ness, so 
you err to assume that it must be const. And as others have already 
pointed out, returning by-value should be done non-const to allow 
certain useful programming idioms. Yes, it also opens the oppotunity to 
create bugs, but when I have the choice between nannyism and freedom, I 
prefer the latter. In the end, returning by-const-value also leads to 
inefficiencies in certain situations.

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

IIRC = If I Remember Correctly, a common abbreviation on the net.

> references on the www.boost.org website), if decisions are taken on a  
> case-by-case basis?

Boost developers are not gods. We sometimes just allow different idioms 
as long as there is no clear problem with one of them. This is a good 
thing as we can see where it breaks or if both idioms are equally good. 
If they are, there is no need for a guideline. I guess you will even 
find proponents of the return-by-const-value camp here and given enough 
supporters that think it's worth the trouble, I might consider a #define 
to enable returning by-const-value. But please realize while this 
technique has its pros, it also has some cons and things are not as 
clear as you suggest.

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

None of this is true and once again your wording is offending. Thus, 
your homework: Show at least *one* example to prove that your 
implementation is better. :)

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

We had examples where the NRVO-correct implementation is better than the 
RVO-version. Note that both versions are still better than your version. 
  You really just claims your implementation is better, but please, show 
us an example! If you are too lazy, I can write up a small example but I 
don't feel like I should do this without you at least trying it. And 
believe me, you can learn a lot of things about compilers and the 
standard :)

> 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,

Nope.

>   b) the optimization can safely be removed.

Nope.

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

I haven't looked into that, will do when I have more time at the weekend.

Regards, Daniel