operators.hpp bugs
I have found a couple of "undocumented features" in operators.hpp: 1) The return values from a lot of the binary operators should be declared const, so that it is not possible to assign to the temporary object that results from the operation. An example: if(!(a/b=c/d))... This statement is obviously a typo. The assignment operator= should have been a comparison operator==. With the current definition of operator/ in operators.hpp, the compiler will not recognize this as an error. Instead, the following will happen: first, the values of a/b and b/c are calculated and placed in temporary objects. Then, the result of a/b is overwritten by the result of c/d. Finally, operator! converts that to a boolean and the if statement acts on it (in other words, the statement is probably interpreted as "if(!c)..."). If, on the other hand, the return value from operator/ is declared constant, the compiler will catch the error, and probably say something like "lvalue required". Much better. 2) I think the "Named Return Value Optimization" code is a misunderstanding. The way I read the excerpts from the C++ standard quoted in the documentation (see http://www.boost.org/libs/utility/operators.htm#symmetry) is as follows: The standard places some limits on when NAMED objects may be optimized away. From this follows implicitly that UNNAMED objects may always be optimized away. Or, to quote from Scott Meyers' "More Effective C++", page 109-110: "However, the fact remains that unnamed objects have historically been easier to eliminate than named objects, so when faced with a choice between a named object and a temporary object, you may be better off using the temporary. It should never cost you more than its named collegue, and, especially with older compilers, it may cost you less". In other words: some compilers may not have Named Return Value Optimization, but most of them have (Unnamed) Return Value Optimization, which means that temporary, unnamed objects are optimized away where possible. So, I think, a better way of writing e.g. operator+ is the good old-fashioned: template <class T> inline const T operator+(const T& lhs,const T& rhs) {return T(lhs) += rhs;}; 3) When using compilers that do not support operators in namespaces (#ifdef BOOST_NO_OPERATORS_IN_NAMESPACE), the operator templates are first placed in the global namespace, and then copied to the boost namespace. The consequence of this is that it is not possible to access the templates with a "using namespace boost;" declaration - trying to do so creates an ambiguity between the stuff in boost and the stuff in the global namespace. Also, of course, the global namespace is "polluted" with quite a lot of extra names. The solution to this problem is to declare all the templates inside a "pseudo-namespace" struct in the global namespace - this also makes it easier to import the templates into boost, as even the Borland compiler now recognizes a "using" declaration. I have uploaded a new version of "operators.hpp" with fixes for above-mentioned problems to the sandbox vault. It has been tested with "rational.hpp" on the Borland 5.5 compiler and the Microsoft version 13.10.3077 compiler (the one that comes with the free Visual Toolkit 2003, and also (I think) with Visual C++ 7.1). I hope that some of you have time to test my new version with other programs and compilers. --- Søren
I have found a couple of "undocumented features" in operators.hpp:
1) The return values from a lot of the binary operators should be declared const, so that it is not possible to assign to the temporary object that results from the operation. An example:
if(!(a/b=c/d))...
Well, operators are generally defined in the same form as predefined operators and thus does not returns const objects. You might look into the book "C++ Coding Standard" by Herb Sutter and Andrei Alexandrescu at items 27 and 55 in particular. If operators returns const objects the many functions should probably also returns const objects. As mentionned in the book, returning const objects disable some usefull construct on temporary objects (like calling a non-const member function) and might break some existing client code. So for consistency, I think that boost operators should returns non-const objects as most librairies do (including probably the STL for complex numbers). Philippe
On Mon, 25 Apr 2005 13:01:58 -0400, Philippe Mori <philippe_mori@hotmail.com> wrote:
You might look into the book "C++ Coding Standard" by Herb Sutter and Andrei Alexandrescu at items 27 and 55 in particular.
If operators returns const objects the many functions should probably also returns const objects.
As mentionned in the book, returning const objects disable some usefull construct on temporary objects (like calling a non-const member function) and might break some existing client code.
So for consistency, I think that boost operators should returns non-const objects as most librairies do (including probably the STL for complex numbers).
Well, according to my guru, that is not how things are supposed to work. See Scott Meyers' "Effective C++" 2nd Edition, page 91. His main point is that user-defined types should behave like the built-in types - meaning that a construct like (a * b) = c; should be illegal, whether a,b, and c are integers, floats or user-defined. So, for consistency, I think that boost operators should return const objects. More generally, he writes (same page): "Having a function return a constant value often makes it possible to reduce the incidence of client errors without giving up safety or efficiency". So yes, I think you are right, many (most) functions should also return const objects. I suppose, though, that there may be situations where it may be of interest to call a non-const member function on a temporary object, although this sounds a bit like sloppy programming to me. In terms of operators.hpp, however, this is rarely the case, as the binary operators are declared inline, meaning that the return values in most cases are constructed right in the target variables. Therefore I think that e.g. x=(a+b).somenonconstfunction(); will rarely be faster than UserType y=a+b; x=y.somenonconstfunction(); You are right about the complex numbers in the STL. And this, of course, makes programming constructs like complex<float> x=((a*b)+=c); possible. But because the complex operators * and + are declared inline, there is no reason to think that this is faster than complex<float> x=(a*b)+c; only harder to read, and non-compatible with built-in types. And it is never faster than complex<float> x=(a*b); x+=c; I do not see any realistic uses of the non-const return values from the binary operators in complex.h. Can you give me an example? And more importantly, are there boost libraries or client libraries that depend on the non-const-ness of the binary operator results in operators.hpp? --- Søren
Well, according to my guru, that is not how things are supposed to work. See Scott Meyers' "Effective C++" 2nd Edition, page 91. His main point is that user-defined types should behave like the built-in types - meaning that a construct like (a * b) = c; should be illegal, whether a,b, and c are integers, floats or user-defined. So, for consistency, I think that boost operators should return const objects.
I have the first edition of that book and it does not seems to talk about returning const objects in that older edition. Item 15 (page 48) says that operator= returns *this. Which item # are you referring at page 91? It seems like some important changes were made between both editions.
You are right about the complex numbers in the STL. And this, of course, makes programming constructs like complex<float> x=((a*b)+=c); possible.
I do not see any realistic uses of the non-const return values from the binary operators in complex.h. Can you give me an example?
Well, I see 4 reasons why in general someone would like to be able to write complex expressions instead of breaking them into multiple statements. The first one would be to avoid declaring the type of intermediate variables which might not always be evident (and in some situations one could accidently uses a compatible type that would add an useless conversion without any warning as it might be the case if someone write complex<double> instead of complex<float>). The cause of this reason is the absence of type deduction in C++ (I know about workaround in Boost) and many have suggested to reuse the auto keyword for that... The second reason is that in some situation we want only one statement or expression if possible. This include objects that are initialized in the initialization-list of a constructor and also in macros that should behave like a statement. The third one is backward compatibility (it will surely breaks someone code) and compatibility with the usual ways of doing things... The forth one is that in case of operator=, the return value is (according to a few sources) a non-const reference... and I was thinking it was a requirement of STL. If this is the case, it would means that we would allows (a = b).do_something(); but not (a + b).do_something(); I think that it is not possible to simulate built-in types at 100% as probably retuning const objects is more restrictive that what happens with built-in type and non-const is less restrictive.
And more importantly, are there boost libraries or client libraries that depend on the non-const-ness of the binary operator results in operators.hpp?
I don't knows and I don't think the change would affect me. Philippe
On Mon, 25 Apr 2005 18:45:03 -0400, Philippe Mori <philippe_mori@hotmail.com> wrote:
Which item # are you referring at page 91? It seems like some important changes were made between both editions.
Sorry, the item is #21: "Use const whenever possible". Yes, there are probably some changes between editions. That may be the reason that the Boost Guidelines explicitly refer to the second edition. I wouldn't know, though. I have never seen the first edition. -- Søren
Which item # are you referring at page 91? It seems like some important changes were made between both editions.
Sorry, the item is #21: "Use const whenever possible". Yes, there are probably some changes between editions. That may be the reason that the Boost Guidelines explicitly refer to the second edition. I wouldn't know, though. I have never seen the first edition.
-- Søren
In edition 1, he mentions it is possible to returns a const object but none of the example are related to that and his examples does not have top-level const qualifier. That is, he uses const char * and not const char * const for the return value. I don't knows about edition 2... In pratice, const is seldom used for return value and for arguments as it provide not as much benefice in those cases as the effect is local. Philippe
Søren Lassen wrote:
On Mon, 25 Apr 2005 18:45:03 -0400, Philippe Mori <philippe_mori@hotmail.com> wrote:
Which item # are you referring at page 91? It seems like some important changes were made between both editions.
Sorry, the item is #21: "Use const whenever possible". Yes, there are probably some changes between editions. That may be the reason that the Boost Guidelines explicitly refer to the second edition. I wouldn't know, though. I have never seen the first edition.
To make the confusion complete, we'll probably have to recommend the 3rd edition shortly. http://www.amazon.co.uk/exec/obidos/ASIN/0321334876/qid=1114715488/sr=8-8/re... -- /Brian Riis
Daniel, I am Cc:'ing you because IIRC you are the maintainer. "Philippe Mori" <philippe_mori@hotmail.com> writes:
I have found a couple of "undocumented features" in operators.hpp:
1) The return values from a lot of the binary operators should be declared const, so that it is not possible to assign to the temporary object that results from the operation. An example:
if(!(a/b=c/d))...
Well, operators are generally defined in the same form as predefined operators and thus does not returns const objects.
You might look into the book "C++ Coding Standard" by Herb Sutter and Andrei Alexandrescu at items 27 and 55 in particular.
If operators returns const objects the many functions should probably also returns const objects.
As mentionned in the book, returning const objects disable some usefull construct on temporary objects (like calling a non-const member function) and might break some existing client code.
So for consistency, I think that boost operators should returns non-const objects as most librairies do (including probably the STL for complex numbers).
It was discussed in the past IIRC and Philippe's rationale basically captures the consensus here. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On Tue, 26 Apr 2005 12:44:19 -0400, David Abrahams <dave@boost-consulting.com> 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. It would be easier to contribute to boost if the programming guidelines on the homepage 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. I have reintroduced the non-const return values for the operators, the new version has been uploaded to the sandbox vault. -- Søren
Søren Lassen <s.lassen@post.tele.dk> writes:
On Tue, 26 Apr 2005 12:44:19 -0400, David Abrahams <dave@boost-consulting.com> 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.
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.
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.
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. -- Dave Abrahams Boost Consulting www.boost-consulting.com
Søren Lassen <s.lassen@post.tele.dk> writes:
On Tue, 26 Apr 2005 12:44:19 -0400, David Abrahams <dave@boost-consulting.com> 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
On Wed, 27 Apr 2005 08:01:36 -0400, David Abrahams <dave@boost-consulting.com> wrote: 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
Søren Lassen <s.lassen@post.tele.dk> writes:
On Wed, 27 Apr 2005 08:01:36 -0400, David Abrahams <dave@boost-consulting.com> wrote:
Søren Lassen <s.lassen@post.tele.dk> writes:
On Tue, 26 Apr 2005 12:44:19 -0400, David Abrahams <dave@boost-consulting.com> 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
?? 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".
I think that the user will expect that the prototype of function match exactly those in table... Also, as far as I understand convertible to T is a requirement for the implementation... and in fact it does says nothing as the requirement are (generally) in the form T temp(*this); temp op= u; // +=, -=, ... and is complete by itself. Philippe
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@boost-consulting.com> wrote:
Søren Lassen <s.lassen@post.tele.dk> writes:
On Tue, 26 Apr 2005 12:44:19 -0400, David Abrahams <dave@boost-consulting.com> 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
Søren Lassen wrote:
The standard places some limits on when NAMED objects may be optimized away. From this follows implicitly that UNNAMED objects may always be optimized away. Or, to quote from Scott Meyers' "More Effective C++", page 109-110: "However, the fact remains that unnamed objects have
BTW, did you read Scott's errata? To quote from it: 109 Both the first and second implementations on this page suffer from the problem that they are returning whatever operator=+ returns. In general, there is no way to know what this is (yes, it's a reference to an object of type T, but a reference to which T object?), hence no way for compilers to optimize away the copy to operator+'s return value. The way to write operator+ such that the return value optimization can be performed is with this body: T result(lhs); result += rhs; return result. taken from <http://www.aristeia.com/BookErrata/mec++-errata_frames.html> Regards, Daniel
On Thu, 28 Apr 2005 20:55:02 +0200, Daniel Frey <d.frey@gmx.de> wrote:
BTW, did you read Scott's errata?
No, I did not. But I now tested the two versions of operators.hpp with a couple of compilers (Microsoft and Borland, that's what I have got up and running at the moment, I think both are non-NRVO), and you are right: your version is 10-20% faster (checked with a wristwatch) for operations on an object containing 11 (I programmed first and counted afterwards, therefore the odd number) 32-bit integers (see object definition below). I have to admit that you are right, and I probably ought to hate that fact. I do not, though, as I would rather live in peace with my fellow human beings in general, and the Boost guys in particular. Sorry, I should probably have intialized the dialogue in a more toned-down manner. On the other hand, I do not feel that the somewhat heated tone of the discussion is all my fault. E.g. when Dave tells me that there are no programming guidelines on the homepage, I did feel sort of snubbed - he, of all persons, should know that the homepage has a link to a set of programming guidelines which at least appears to be official Boost policy. Anyway, it may be a good idea to include e.g. a reference to Scott's errata, and to the other books mentioned during the discussion on the guidelines page. Likewise, when there is a discussion about e.g. the const-ness of return values, it may be worth the effort to condense some of the points into a simple text that gets a link on the Guidelines page. -- Søren P.S.: Here is my test program. My version of operators.hpp takes about 18 seconds with the Borland compiler and 23 with the Microsoft compiler, whereas the official version takes about 15, resp. 20 seconds to run, all very approximately (no need to count the milliseconds, the assembly output also contains more "mov" instructions in my version): #include <boost/operators.hpp> class rvo:boost::addable1<rvo>{ int a,b,c,d,e,f,g,h,i,j,k; public: rvo(int x=0){a=b=c=d=e=f=g=h=i=j=k=x;} rvo& operator+=(const rvo &r){ a+=r.a; b+=r.b; c+=r.c; d+=r.d; e+=r.e; f+=r.f; g+=r.g; h+=r.h; i+=r.i; j+=r.j; k+=r.k; return *this;} }; int main(){ { rvo a(1),b(2),d(4),e(5),c; for(int i=0;i<100000000;++i) c=a+b+d+e; }
Søren Lassen <s.lassen@post.tele.dk> writes:
Sorry, I should probably have intialized the dialogue in a more toned-down manner. On the other hand, I do not feel that the somewhat heated tone of the discussion is all my fault. E.g. when Dave tells me that there are no programming guidelines on the homepage, I did feel sort of snubbed -
Get over it, it's true.
he, of all persons, should know that the homepage has a link to a set of programming guidelines
Are you kidding? Why "of all persons" should I remember every link on the homepage, and know that's what you're talking about when you refer to "guidelines on the home page?"
which at least appears to be official Boost policy.
There is no guideline about constness of return types. You extrapolated from a reference to a book the idea that Boost takes everything in that book to be an absolute rule. Look, you show up with an almost completely misguided and hasty post about "bugs" in a library, then you try to claim that Boost isn't following its own programming guidelines, and then haul out http://www.boost.org/more/lib_guide.htm#Rationale_rationale to deliver what feels to me like a verbal bludgeoning with our own words. At that point I don't think you should expect anyone to bend over backwards to be understanding. So, no "the somewhat heated tone of the discussion" is not all your fault. If I were a perfectly enlightened being, I'm sure I wouldn't be annoyed and I would have found some way to gently and patiently correct all of your misperceptions. Unfortunately for everyone involved, I'm just human like everyone else. And, while I appreciate that you're willing to take some responsibility, "sorry, I shouldn't have done that... but it's really Dave's fault too" doesn't amount to much of an apology.
Anyway, it may be a good idea to include e.g. a reference to Scott's errata, and to the other books mentioned during the discussion on the guidelines page. Likewise, when there is a discussion about e.g. the const-ness of return values, it may be worth the effort to condense some of the points into a simple text that gets a link on the Guidelines page.
Patches are welcome.
-- Søren
P.S.: Here is my test program. My version of operators.hpp takes about 18 seconds with the Borland compiler and 23 with the Microsoft compiler, whereas the official version takes about 15, resp. 20 seconds to run, all very approximately (no need to count the milliseconds, the assembly output also contains more "mov" instructions in my version):
What we really need, as I have requested several times already, is a test program for the problem your other patch addresses. I mean the one that still might be valid, and that fixes the compilation failure on some compilers. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On Thu, 28 Apr 2005 21:13:39 -0400, David Abrahams <dave@boost-consulting.com> wrote:
What we really need, as I have requested several times already, is a test program for the problem your other patch addresses. I mean the one that still might be valid, and that fixes the compilation failure on some compilers.
OK, here are two examples that compile on most compilers, but not Borland 5.5 (are there others that do not accept inline friend functions in namespaces?), as fully fledged C++ programs. Example1.cpp: #include <boost/operators.hpp> using namespace boost; class a:addable1<a>{ public: a& operator+=(const a&){return *this;}; }; int main(){ } Example2.cpp: #include <boost/operators.hpp> bool addable1; int main(){ } If you think my proposed solution is usable, I shall be happy to write a version of operators.hpp that implements it - but I shall be away for holidays the next 5 weeks, so it will not be immediately. Feel free use whatever you like from my example in the sandbox vault, if you prefer. Sorry again for my too literal interpretation of the gudelines page - I have probably spent too many years in corporate DP departments, where programming guidelines are acknowledged, detailed, updated frequently, widely disseminated, and quite often adhered to or even enforced - at least in the sense that you have to come up with an explanation when you deviate from the guidelines. I am not saying that this is a better way of doing things, just that I am used to doing things differently. -- Søren
Søren Lassen <s.lassen@post.tele.dk> writes:
On Thu, 28 Apr 2005 21:13:39 -0400, David Abrahams <dave@boost-consulting.com> wrote:
What we really need, as I have requested several times already, is a test program for the problem your other patch addresses. I mean the one that still might be valid, and that fixes the compilation failure on some compilers.
OK, here are two examples that compile on most compilers, but not Borland 5.5 (are there others that do not accept inline friend functions in namespaces?), as fully fledged C++ programs.
<snip> Thanks a lot, Søren. Daniel Frey, the library's maintainer, will be looking at this stuff. -- Dave Abrahams Boost Consulting www.boost-consulting.com
Hi Søren, I hope you enjoyed your holidays :) I CC'ed you to make sure this won't get lost. Søren Lassen wrote:
OK, here are two examples that compile on most compilers, but not Borland 5.5 (are there others that do not accept inline friend functions in namespaces?), as fully fledged C++ programs. Example1.cpp: #include <boost/operators.hpp> using namespace boost; class a:addable1<a>{ public: a& operator+=(const a&){return *this;}; };
int main(){ }
Example2.cpp: #include <boost/operators.hpp> bool addable1; int main(){ }
If you think my proposed solution is usable, I shall be happy to write a version of operators.hpp that implements it - but I shall be away for holidays the next 5 weeks, so it will not be immediately. Feel free use whatever you like from my example in the sandbox vault, if you prefer.
I meanwhile looked at your code (only the part for the above) and there are some points that bother me: - Looking at boost/config/*, I can see that there are 4 compilers affected by BOOST_NO_OPERATORS_IN_NAMESPACE, luckily only older versions. It seems that all current compilers don't need the work-around. This means to me that we have a working work-around and all changes are dangerous as they can break existing platforms. New users on these platforms can probably live with the inconveniences, ports to these platforms should IMHO be rare. Users of the old platforms are used to the pain (I still have to use GCC 2.95.2 (yes, .2!) in the company, I know what I'm talking about ;). - The first example is IMHO bad style. Using "using" in such a global context is explictly asking for trouble. My experience shows that "using" shall be used rarely and as local as possible, preferable at function scope or when assembling a class' interface. This might not be your point in the above example, but I don't think the first one is really "worth" fixing - YMMV. And obviously it would help to find others that find your fix worth the trouble. - You work-around is illegal. You are not allowed to apply a using-declaration to a template-id (see 7.3.3/5). That given, I won't apply any fix before the code has been tested with all affected compilers (anyone?) as even if it works for Borland, chances are that the other compilers do yell at the illegal code. Or maybe you want to reconsider writing a fix for Borland-only? Still this doesn't seem worth the effort since the gain is IMHO quite small. - The second example you gave above could IMHO be fixed in other ways - like renaming the classes in the global namespace to e.g. BOOST_addable1, BOOST_addable2, etc. and provide new, correctly named classes in boost like the "combined operator classes". Example: // global namespace template <class T, class B = ::boost::detail::empty_base> struct BOOST_incrementable : B { friend T operator++(T& x, int) { incrementable_type nrv(x); ++x; return nrv; } private: // The use of this typedef works around a Borland bug typedef T incrementable_type; }; namespace boost { // Forwarder for incrementable template <class T, class B = ::boost::detail::empty_base> struct incrementable : ::BOOST_incrementable< T, B > {}; } The BOOST_-prefix should prevent all name clashes (BOOST_* is "reserved for us anyway, right? ;) and the combined operator classes are known to work, so adding some more of them (stupid forwarders, but anyway) is far less intrusive than your patch. Both of your examples above should be solved by this approach AFAICS. As an added bonus, we could get rid of all "using"s, which is a good thing as "using" is known to be buggy on some compilers. Of course this is just an idea that I never tested. If you or other think it's worth a try, I can prepare a patch for people to test. Comments? Regards, Daniel
Daniel Frey <d.frey@gmx.de> writes:
Of course this is just an idea that I never tested. If you or other think it's worth a try, I can prepare a patch for people to test. Comments?
If anyone cares about having the problems fixed, I think that's a great approach. -- Dave Abrahams Boost Consulting www.boost-consulting.com
participants (2)
-
Brian Ravnsgaard Riis -
Daniel Frey -
David Abrahams -
Philippe Mori -
Søren Lassen