Awesome! Thanks for this. I will fix the code in GitHub.
-greg
> On 04 Nov 2013, at 19:09, Jack Howarth <howa...@bromo.med.uc.edu> wrote:
>
> Howard Hinnant, one of the Apple libc++ maintainers, kindly analyzed the
> compilation failure of Timer.cpp and had the
> following comments...
>
> --------------------------------------------------------------------------------------------------------------------------------------------
> The problem is here:
>
> class Dict {
> public:
> typedef std::map<const std::string, boost::any> DataType; // <-------
>
> When the copy assignment operator of Dict does:
>
> _data = other._data;
>
> (where _data is a DataType), then libc++ tries to assign to a *const*
> std::string, and triggers the error.
>
> The fix is simple: Remove the const from the std::string in DataType:
>
> typedef std::map<std::string, boost::any> DataType;
>
> This is an understandable error when upgrading from a C++03-based std::lib to
> a C++11-based std::lib. You've stumbled across one of the very
> few places that the committee broke API between C++03 and C++11. And the API
> is broken for a very good reason: A very significant
> performance optimization opportunity for [multi]map.
>
> Background:
>
> The C++98/03 [multi]map/set containers did not require the contained element
> to be assignable, even for the copy assignment operator of these
> containers. Instead the implementation was supposed to do an erase, or clear
> of the lhs, followed by an insert for every element in the rhs.
>
> In C++11 23.2.4 [associative.reqmts]/p7 (and there's a similar paragraph for
> the unordered containers) transfers the requirements on
> value_type (the pair<const key_type, mapped_type>), directly to the key_type
> and mapped_type, specifically calling out CopyAssignable as an
> example.
>
> This allows the map copy assignment operator to recycle nodes (assign the
> key_type and mapped_type within the nodes) as part of the assignment
> process, in addition to using erase/insert. In your example, if _data.size()
> == other._data.size(), and if all strings in other._data are
> short, the assignment could be done with *zero* trips to the heap (*very
> fast*). In contrast, every erase, and every insert results in at
> least one deallocation and allocation respectively.
>
> I'm not positive but I believe libc++ may be the only std::lib which
> currently implements this optimization, and so compiling against other
> std::libs would not trigger this error.
>
> You can blame me for this API breakage in C++11. I believed (and still
> believe) the cost in breakage is worth the performance benefit. I see
> const-qualified types in containers very rarely (strictly speaking I think
> they are not even allowed, but I would have to double check that).
>
> Also libc++ follows the C++11 rules even with -std=c++03. The philosophy of
> libc++ is that there are already plenty of C++03 std::libs.
> libc++ emulates a C++11 environment, even when -std=c++03. This is meant to
> aid clients in transition from C++03 to C++11. For example
> std::shared_ptr and std::unordered_map are available even in -std=c++03.
>
> If for some reason taking the const out of the DataType typedef is not
> desirable, you can easily emulate the C++03 behavior (but giving up the
> performance optimization) by replacing:
>
> Dict &operator=(const Dict &other) {
> _data = other._data;
> return *this;
> };
>
> with:
>
> Dict &operator=(const Dict &other) {
> if (this != &other) {
> _data.clear();
> _data.insert(other._data.begin(), other._data.end());
> }
> return *this;
> };
>
> And this:
>
> Dict(const Dict &other) {
> _data = other._data;
> };
>
> with:
>
> Dict(const Dict &other)
> : _data(other._data)
> {};
>
>
> This latter change should be made whether or not you take the const out of
> DataType. It is inefficient to default construct a data member
> just to assign it a new value. Better to construct it directly to the
> desired value.
> -------------------------------------------------------------------------------------------------------------------------------------------
>
> So the trivial patch....
>
> --- RDKit_2013_06_1/Code/RDGeneral/Dict.h.orig 2013-11-03
> 13:22:27.000000000 -0500
> +++ RDKit_2013_06_1/Code/RDGeneral/Dict.h 2013-11-03 13:22:54.000000000
> -0500
> @@ -32,7 +32,7 @@
> //!
> class Dict {
> public:
> - typedef std::map<const std::string, boost::any> DataType;
> + typedef std::map<std::string, boost::any> DataType;
> Dict(){
> _data.clear();
> };
>
> ...allows RDKit_2013_06_1 to completely compile under Xcode 5.0.1 and libc++
> on Mac OS X 10.9.
> Jack
>
> ------------------------------------------------------------------------------
> Android is increasing in popularity, but the open development platform that
> developers love is also attractive to malware creators. Download this white
> paper to learn more about secure code signing practices that can help keep
> Android apps secure.
> http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
> _______________________________________________
> Rdkit-discuss mailing list
> Rdkit-discuss@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
Rdkit-discuss mailing list
Rdkit-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
