[TR1 Review] TR1 Formal Review Beginning
The formal review of the Boost TR1 Library by John Maddock begins today, Saturday, September 24th, and runs through Wednesday, October 5th. The TR1 library provides an implementation of the C++ Technical Report on Standard Library Extensions. This library does not itself implement the TR1 components, rather it's a thin wrapper that will include your standard library's TR1 implementation (if it has one), otherwise it will include the Boost Library equivalents, and import them into namespace std::tr1. Functionality supported includes: * Reference Wrappers * Smart Pointers * Class template result_of. * Function template mem_fn. * Function Object Binders. * Polymorphic function wrappers. * Type Traits. * Random Number Generators and Distributions. * Tuples. * Tuple Interface to std::pair. * Fixed Size Array. * Hash Function Objects. * Regular Expressions. * Complex Number Algorithm Overloads. * Complex Number Additional Algorithms. Functionality in TR1 but not yet available: * Mathematical Special Functions. * Unordered Associative Set (Hash Table). * Unordered Associative Map (Hash Table). * C99 C language additions. Additional information including online documentation is available at http://freespace.virgin.net/boost.regex/tr1/index.html The complete TR1 lib source and docs can be downloaded from http://freespace.virgin.net/boost.regex/tr1/tr1-20050816.zip Please help Boost by posting your formal review of this library. Your comments may be brief or lengthy, but basically the Review Manager needs your evaluation of the library. If you identify problems along the way, please note if they are minor, serious, or showstoppers. Here are some questions you might want to answer in your review: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. --Beman Dawes, Review Manager
On Sep 24, 2005, at 12:54 PM, Beman Dawes wrote:
The formal review of the Boost TR1 Library by John Maddock begins today, Saturday, September 24th, and runs through Wednesday, October 5th.
I am strongly in favor of accepting this library. I think it is very important to provide a transition path from Boost to vendor implementations of TR1, and this is it. ----Effort---- I spent an hour or two reading through the documentation and perusing the code, but didn't actually compile anything because I "know" it works. At one point I had started to write a Boost.TR1 myself, so I had a pretty good idea how the implementation would look, and it looks just like that. Plus, John has a history of solid and well-maintained libraries, and I don't expect that this would be any exception. ----Overall Comments---- - We should find a way to strongly encourage people to help make Boost's components meet the requirements in TR1. Making the "_tricky" tests show up as failures in the regression tests would be a great start. ----Documentation Comments---- - Why must users explicitly enable use of the underlying TR1 library, instead of explicitly disabling it? I ask because one day I'd like to stop maintaining Boost.Function, and remove it from Boost entirely. It's been standardized in a TR, vendors are implementing it, and in a sense our work is done once all of the commonly-used standard library implementations have it. I see Boost.TR1 as the migration path from Boost to standards, and I think we should push users along that path as forcefully as we can without them pushing back. - "Configuration" bullets for each library: shouldn't Boost.Config define BOOST_HAS_TR1_xxx, not the user? - "Polymorphic function wrappers", standard conformity: Should also add a sentence: "The member function target() can only access pointer-to-member targets when they have been wrapped in mem_fn." - Mathematical Special Functions, hash tables, etc.: Should these go under a seperate heading for TR1 parts *not* supported by Boost? A quick glance through the documentation of the Boost.TR1 library makes it seem like they are supported. - "Tuples", Standard Conformity: Replace "withy" withy "with" :) - "Testing": Typo "third part implementations". ----Code Comments---- - The redirecting headers (boost/tr1/tr1/memory) check __GNUC__ to determine whether they should #include_next or use BOOST_TR1_STD_HEADER; we should probably have a BOOST_HAS_INCLUDE_NEXT macro to indicate #include_next support. - The redirecting headers themselves use some interesting preprocessor logic, but it looks correct to me. I'd tried something similar, although I missed the case of circular #includes inside the vendor's headers when I did it. - The header tr1/detail/config.hpp uses BOOST_HAS_TR1 to turn on support for "everything", where "everything" consists of BOOST_HAS_TR1_REFERENCE_WRAPPER and BOOST_HAS_TR1_SHARED_PTR. Should there be more? Great work, John! Doug
- We should find a way to strongly encourage people to help make Boost's components meet the requirements in TR1. Making the "_tricky" tests show up as failures in the regression tests would be a great start.
I'm hoping we get more of our runtime tests to work with std::tr1:: names as well as with boost:: names as well. The current tr1 tests do this for type traits, but that's it so far....
- Why must users explicitly enable use of the underlying TR1 library, instead of explicitly disabling it?
Ah, a philisophical question :-) I choose BOOST_HAS_XXXX rather than BOOST_NO_XXXX since TR1 is not actually part of the standard, it's an optional extension.
- "Configuration" bullets for each library: shouldn't Boost.Config define BOOST_HAS_TR1_xxx, not the user?
Yes, that's the intention, there just aren't any changes needed to Boost.Config yet (but hold on does gcc-4 have any TR1 components yet?)
- "Polymorphic function wrappers", standard conformity: Should also add a sentence: "The member function target() can only access pointer-to-member targets when they have been wrapped in mem_fn."
- Mathematical Special Functions, hash tables, etc.: Should these go under a seperate heading for TR1 parts *not* supported by Boost? A quick glance through the documentation of the Boost.TR1 library makes it seem like they are supported.
Understood, I've never really been sure how to handle the unsupported parts, well actually they are supported if the underlying standard library supports them. But maybe they need to be moved to their own table of contents to make this clear?
- "Tuples", Standard Conformity: Replace "withy" withy "with" :)
- "Testing": Typo "third part implementations".
----Code Comments----
- The redirecting headers (boost/tr1/tr1/memory) check __GNUC__ to determine whether they should #include_next or use BOOST_TR1_STD_HEADER; we should probably have a BOOST_HAS_INCLUDE_NEXT macro to indicate #include_next support.
Good point.
- The redirecting headers themselves use some interesting preprocessor logic, but it looks correct to me. I'd tried something similar, although I missed the case of circular #includes inside the vendor's headers when I did it.
Yep, it was surprising hard to do, I'm not even sure that it is right in every possible case, but it's probably as close as we can get for now: the difficult part is we need to have one std lib header "unwrapped" so that Boost.Config can work, but all the others *must* be wrapped, even those that aren't changed in TR1 in case they #include something that is modified by TR1 (which would lead to horrid cyclic dependencies if we don't take steps to stop it).
- The header tr1/detail/config.hpp uses BOOST_HAS_TR1 to turn on support for "everything", where "everything" consists of BOOST_HAS_TR1_REFERENCE_WRAPPER and BOOST_HAS_TR1_SHARED_PTR. Should there be more?
Um, embarrassment :-( Maybe one or two more ;-) John.
On Sep 26, 2005, at 5:02 AM, John Maddock wrote:
- "Configuration" bullets for each library: shouldn't Boost.Config define BOOST_HAS_TR1_xxx, not the user?
Yes, that's the intention, there just aren't any changes needed to Boost.Config yet (but hold on does gcc-4 have any TR1 components yet?)
GCC 4.0 has "experimental" versions of array, bind, shared_ptr, result_of, reference_wrapper, tuple, type_traits, unordered_map/ unordered_set. I think most of them are pretty sound, although I think "bind" still has some bugs with handling nested bind expressions.
- Mathematical Special Functions, hash tables, etc.: Should these go under a seperate heading for TR1 parts *not* supported by Boost? A quick glance through the documentation of the Boost.TR1 library makes it seem like they are supported.
Understood, I've never really been sure how to handle the unsupported parts, well actually they are supported if the underlying standard library supports them. But maybe they need to be moved to their own table of contents to make this clear?
I think that is a good idea. Doug
John Maddock wrote:
- We should find a way to strongly encourage people to help make Boost's components meet the requirements in TR1. Making the "_tricky" tests show up as failures in the regression tests would be a great start.
I'm hoping we get more of our runtime tests to work with std::tr1:: names as well as with boost:: names as well. The current tr1 tests do this for type traits, but that's it so far....
The hash library has some support for testing the relevant part of TR1 - although I haven't really tested it, so there are probably some mistakes. I use a macro for the namespace, and disable the tests for the extensions (which make up most of the tests) when TEST_STD is set. Come to think of it, I added the examples to the tests, so it won't work at the moment. I'll create a target which doesn't include those. Daniel
Beman Dawes wrote: ...
I spent a couple of hours reading and commenting documentation. I also looked into boost/tr/functional.hpp and boost/tr1/memory.hpp. My review covers documentation only. Header Include Style --------------------
#include <boost/tr1/memory.hpp>
It's fine with me even in spite of one small inconsistency. Why the path starts with boost although the stuff is in namespace std? Reference Wrappers ------------------ Smart Pointers -------------- No dot in the end. It's inconsistent with other subsections of 'TR1 By Subject' section. Random Number Generators and Distributions. ------------------------------------------- Mathematical Special Functions. ------------------------------- My knowledge of these libraries is very limited so I skip them. Tuples. -------
6.1 Tuple types Containers 80 This sentense doesn't begin with C++ comments and sounds rather strange to me.
Interoperability withy std::pair is only partially supported.
1. Someone already noticed a typo (withy) and 2. "partially supported" doesn't give any clue what's not supported. Hash Function Objects. ---------------------- 1. No reference to Boost.Hash documentation. 2. It's not documented that boost version of std::tr1::hash is defined on a wider set of types then required by 6.3.3/1. It doesn't have to be documented but boost::hash has some side effects which worth mentioning in "Standard Conformity" section. For example, boost::hash is valid for enums even if hash_value is not specialized (BTW, why enums are not listed in 6.3.3?). I vote to accept Boost.TR1. -- Alexander Nasonov
John Maddock wrote:
Thanks for proofing the docs, I'll fix all those issues you spotted.
I suppose you're not going to completely fix what I wrote about boost::hash in TR1. I was unclear about this section. What I was trying to say is that a documentation of Boost.Hash should be changed. TR1 should just emphasize major differences and have a link. Let's redirect Boost.Hash issues to Daniel James: 1. Original post http://lists.boost.org/Archives/boost/2005/09/94385.php 2. Adding new has_value may have side effects, for example, (a) hash<T> may compile for not customized type T although T doesn't necesarily have properly defined equality function; or (b) it may introduce overload resolution ambiguities, in worst case, hash<type_from_6_3_3_1> may stop working. 3. A link to books.hpp leads to books.cpp (note h and c) http://www.boost.org/doc/html/hash/custom.html. Currently, I can't access cvs and fix it myself. -- Alexander Nasonov
Alexander Nasonov wrote:
It doesn't have to be documented but boost::hash has some side effects which worth mentioning in "Standard Conformity" section. For example, boost::hash is valid for enums even if hash_value is not specialized
To be honest, this never occured to me. Although, I guess it is implied by careful reading of the reference documentation (ie. more careful than my writing). It actually deals with enums very nicely and I wouldn't be suprised if Peter (who wrote the original design) had anticipated this. I'll think about this and add something to the documentation (for 1.34).
2. Adding new has_value may have side effects, for example, (a) hash<T> may compile for not customized type T although T doesn't necesarily have properly defined equality function; or (b) it may introduce overload resolution ambiguities, in worst case, hash<type_from_6_3_3_1> may stop working.
I don't think this is nearly as bad as you think. Any type from TR1 will only stop working if you declare hash_value in the boost or std namespace - which you really shouldn't do. If a type has a hash_value function but no equality then boost::hash will compile for it - but a hashed container won't, because it will require the equality function. So the only time will boost::hash will work is if it's used in another context where equality isn't required. I think the only potential problem is if a user defined type has a hash_value function which means something different. A programmer using this type with Boost.MultiIndex or the proposed unordered containers could be completely ignorant of boost::hash as it's used by default. Although, this seems pretty unlikely to me as 'hash_value' is a fairly unambiguous name. And of course, the same problems could happen for std::equal_to, if operator== is overloaded. There's been a long debate about this sort of issue concerning Boost.Range (I'm afraid I lost track of it a while ago. I really should try to catch up, but time is always a problem). In case you're interested, I did at one point consider having a different version for Boost.TR1: http://lists.boost.org/Archives/boost/2005/02/79892.php http://lists.boost.org/Archives/boost/2005/02/79950.php It wouldn't be too hard to provide a 'strict mode' activated by a macro, but I don't really think it would be that useful.
3. A link to books.hpp leads to books.cpp (note h and c) http://www.boost.org/doc/html/hash/custom.html. Currently, I can't access cvs and fix it myself.
OK, I'll fix that soon (for 1.33.1). Daniel
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Beman Dawes | Sent: 24 September 2005 18:55 | To: Boost mailing list; Boost-users mailing list; | Boost-announce mailing list | Subject: [boost] [TR1 Review] TR1 Formal Review Beginning | | The formal review of the Boost TR1 Library by John Maddock | Functionality in TR1 but not yet available: | * Mathematical Special Functions. | * C99 C language additions. :-(( - I got really (over-)excited when I first read about this! These are really important things for many people, even if they don't realise it yet! | My formal review of this library. | * What is your evaluation of the design? I do not _fully_ understand the implications of the design but I can't see any problems, apart from the complexity of the many namespaces which will confuse 'novices' it if doesn't 'wrap' as smoothly as advertised. | * What is your evaluation of the implementation? Haven't tried it yet. | * What is your evaluation of the documentation? Lacking a _simple_ example? But generally OK. | * What is your evaluation of the potential usefulness of | the library? Should ease the transition. | * Did you try to use the library? No yet. BUT I _HAVE_ been working on some of the missing math functions by a 'wrap' of Stephen Moshier's Cephes C library. I have posted a 'work in progress' for those interested in math functions, specially for comment, at http://www.hetp.u-net.com/public/unitTestFunc1.cpp I have used the Boost Test suite to provide some examples of testing some of the C99, TR1 and proposed 'TR2' math functions. The actual function code is the Cephes C float and double library. (Since I only have MSVC 8.0 beta 2, where long double == double, I have ducked providing long double, except to provide the less accurate C double result. This is of course, still Standard compliant because, like sin, cos, log etc, no accuracy is specified). Specifying accuracy remains a difficult matter. Some, but I fear not all, of the C99 and TR1 functions can be provided by the Cephes library. I would welcome views on how to deal with the ones that are missing (until such time as they be implemented). One could just return 0. and still be Standard Compliant, but would throwing an exception be more helpful? If some _exactly_ how and what? Or should it fail at link time? I am unclear how best to mesh this with John's wrapper. | * How much effort did you put into your evaluation? An hour reading the documentation. | * Are you knowledgeable about the problem domain? No. | * Do you think the library should be accepted as a Boost library? Yes. Paul Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB +44 1539 561830 +44 7714 330204 mailto: pbristow@hetp.u-net.com www.hetp.u-net.com
Some, but I fear not all, of the C99 and TR1 functions can be provided by the Cephes library. I would welcome views on how to deal with the ones that are missing (until such time as they be implemented). One could just return 0. and still be Standard Compliant, but would throwing an exception be more helpful? If some _exactly_ how and what? Or should it fail at link time?
Compile time preferably: generally speeking the earlier the error the better. Which ones are missing? Which are the highest priority to implement (assuming someone gets the time!) ? John.
BUT I _HAVE_ been working on some of the missing math functions by a 'wrap' of Stephen Moshier's Cephes C library.
I have posted a 'work in progress' for those interested in math functions, specially for comment, at
Hi, Paul, going over the TR1 review comments I've only just looked at this (sorry), I noticed you're using: BOOST_CHECK((std::tr1::beta(3., 4.) - 1.6666666666666666666666666666666666666667E-2) < numeric_limits<double>::epsilon()); There are probably a couple of issues: firstly you should feed the (found_value-expected_value) part into std::fabs, otherwise any arbitrarily small (or negative!) result will pass the test. Next the < comparison should be a <= since the only way that std::fabs(found_value-expected_value) < numeric_limits<double>::epsilon() can be true is if found_value == expected_value when you think about it :-) The way I handled this in the complex number additions in the TR1 submission was to: Define an acceptable fudge factor as a multiple of numeric_limits<T>::epsilon(), I'd be very surprised if all the special functions are actually accurate to one epsilon ? Then convert the fudge factor to a percentage (multiply by 100) and use BOOST_CHECK_CLOSE (that way you get both expected and found values printed out if there is an error), see libs/math/test/log1p_expm1_test.cpp for some examples. I haven't looked into it yet, but I believe there are some platforms where epsilon is artificially small (for long double on MacOS X), so using numeric_limits<T>::digits() to create your own epsilon may actually be the way to go for testing. HTH, John.
Here's my lengthy review of Boost.TR1
* What is your evaluation of the design?
Good. At one point I thought this library would provide its contents in boost::tr1, as equivalent but more portable / widely available versions of the TR1 libraries, but I'm glad to see it puts everything in std::tr1 instead. I think it's important, so users get used to that. Doug raised the question of whether the native implementations should be used by default, but I think that can be done bit by bit as more native implementations appear and Boost.Config learns which platforms provide which parts. One thing I think is missing is a macro to say "use as much as possible from my native compiler/library, but use Boost versions of the rest." This matters e.g. if a user trusts their compiler vendor more than Boost (maybe because they have a support contract and know they'll get preferential treatment, or they just don't like the Boost website ;-) AFAICT a user would have to manually define the right BOOST_HAS_TR1_XXX macros for their compiler (which would be tedious to define for cross-platform code.) Boost.Config should be taught which compilers provide which parts so users just define one macro. Again, this can come later (and I'll try to help with the GCC parts.)
* What is your evaluation of the implementation?
Good. I like the option of <boost/tr1/memory.hpp> or <memory> (with suitable include path settings.) This gives other Boost headers a consistent way to use them and users the choice. I don't think the restriction on putting the headers in a dir called "include" matters. I like that the relatively fine-grained macros let you pick and choose between the Boost versions and native ones. This would let you use the lock-free shared_ptr from Boost with a vendor's result_of, which might take advantage of some compiler trick that Boost can't use, or whatever other criteria matter to you. The pre-processor tricks to ensure the right things are included are clever but not too complex and without totally obfuscating the affected code (no more so than some other parts of Boost.) The headers will have to be changed with care, but I certainly can't see any easier way of implementing it. Will this library ever go away or will it have to be supported forever? I assume it will stay, so that dead platforms such as MSVC6 can use std::tr1, so I wonder whether having it around permanently might complicate maintenance of other parts of Boost. It probably doesn't matter: if anything in the current Boost.TR1 design causes problems with later native TR1 implementations or parts of Boost it could be changed if it needs to be. A worse consequence of this would be that the TR1 features have to stay in Boost - so Doug has to continue to maintain Boost.Function! Maybe Boost.Function and Boost.TR1 (and the rest) would have to stay until some "Boost v2" that drops support for pre-TR1 compilers.
* What is your evaluation of the documentation?
Very good. It might be useful to list all the BOOST_HAS_TR1_XXX macros on the Configuration page, so you can glance at them all at once (most people will know what components each macro relates to.) Otherwise you have to read through a lot of text on the "TR1 By Subject" page to see what's available. Typo in usage.html says "doing will cause them to cease working." (not "doing so")
* What is your evaluation of the potential usefulness of the library?
Very useful indeed. It gives a portable way to use tr1 on any platform that Boost supports. It would be silly for e.g. boost::regex to compete with std::tr1::regex by being incompatible, or trying to discourage use of the standard versions. This lib helps them maintain close compatibility (by using one to provide the other) and allows users to be agnostic about which they use. Boost has served its purpose of developing these libraries and now can gracefully hand over to the compiler vendors to provide them. This library also helps users to write portable code by providing another TR1 implementation to compile and test against.
* Did you try to use the library? With what compiler? Did you have any problems?
Yes, with GCC 3.4, 4.0 and 4.1 and I had problems when I tried to use the native TR1 bits from GCC 4. I've let John know about this already. Basically GCC doesn't provide a compiler switch to use the TR1 headers, and adding $GCC_ROOT_DIRECTORY/include/c++/4.X.Y/tr1 to your include path doesn't work, due to some bugs I've found in the GCC headers. The only way to use the headers is to include them as e.g. <tr1/memory> The fix is simple and I think John is going to work it into the BOOST_TR1_STD_HEADER() macro. I'll investigate the bugs in the GCC headers tomorrow and they'll get fixed by someone. I'll also try to document GCC's TR1 support.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
About waist-deep study. Not very in-depth, but it's quite a small library. I spent a couple of evenings reading and testing it.
* Are you knowledgeable about the problem domain?
Yes, reasonably. I'm familiar with Boost.Config and the tricks done to include the right things in the right places. I've followed the progress of the TR1 report and helped with parts of GCC's implementation.
* Do you think the library should be accepted as a Boost library?
Yes.
As Beman pointed out I should have replied to this before - basically I just agree with everything - but here we go anyway: Jonathan Wakely wrote:
Here's my lengthy review of Boost.TR1 Doug raised the question of whether the native implementations should be used by default, but I think that can be done bit by bit as more native implementations appear and Boost.Config learns which platforms provide which parts.
I think by and large the native version *has* to be used by default, otherwise the Boost and std versions end up competing for the std::tr1 namespace. Teaching Boost.Config about various std libs is obviously an ongoing process. One thing I must investigate is whether it's possible to rename std::tr1 a little like STLport does so that the Boost verions can be used even when std versions are available.
One thing I think is missing is a macro to say "use as much as possible from my native compiler/library, but use Boost versions of the rest." This matters e.g. if a user trusts their compiler vendor more than Boost (maybe because they have a support contract and know they'll get preferential treatment, or they just don't like the Boost website ;-)
AFAICT a user would have to manually define the right BOOST_HAS_TR1_XXX macros for their compiler (which would be tedious to define for cross-platform code.) Boost.Config should be taught which compilers provide which parts so users just define one macro. Again, this can come later (and I'll try to help with the GCC parts.)
Yep, the library is all config stuff really, it's a matter of keeping Boost.Config up to date with new releases.
* What is your evaluation of the documentation?
Very good. It might be useful to list all the BOOST_HAS_TR1_XXX macros on the Configuration page, so you can glance at them all at once (most people will know what components each macro relates to.)
Yep, will do, or else add them to the Boost.Config docs.
Typo in usage.html says "doing will cause them to cease working." (not "doing so")
Well spotted, thanks. Thanks for the review, John.
participants (7)
-
Alexander Nasonov -
Beman Dawes -
Daniel James -
Douglas Gregor -
John Maddock -
Jonathan Wakely -
Paul A Bristow