Re: [Quantlib-dev] c++0x

[Mike S. <ma...@gm...>]

In regards to keeping the auto_ptr constructor available, I think the
"#define unique_ptr auto_ptr" macro confused me a little bit. I've had a
chance to look into the code today after your feedback, and things are a
lot clearer now.

I've looked into replacing that with a templated typedef, as described by
Herb Sutter at http://www.gotw.ca/gotw/079.htm. Ironically, this would not
be necessary with other new C++11 features, but if it were possible to rely
on those being present we could just use unique_ptr all the time. There
shouldn't be a need to keep the Clone(std::auto_ptr<T>); constructor around
and the templated typedef makes the implementation cleaner while also
preventing quantlib from redefining unique_ptr in client code.

To answer the second question about forcing client code to call std::move()
to transfer ownership of the pointer, that is exactly correct.
Moving from ptrA to ptrB means that any accesses of ptrA besides
reassignment and destruction are undefined behavior.  std::move() appearing
in client code acts as a sort of flag or barrier warning programmers of
this circumstance, so it's unexpected to hide that in a function that takes
a reference. For more information, a good starting point would be this
stack overflow question: http://stackoverflow.com/q/3451099/1181561.

I've committed the two changes I've described to a branch on github if
you'd like to take a look:
https://github.com/masrtis/quantlib/commit/e3b0f3a3ee2c245f3feb60d998687ebcf6e32bd2

Mike


On Fri, May 31, 2013 at 2:06 PM, Peter Caspers <pca...@gm...>wrote:

> Hi Mike,
>
> thank you for your comments. My original goal was to completely get rid
> of auto_ptr (since depricated), therefore I removed the constructor
> taking an auto_ptr. From what you write below I understand that we
> should keep it, but I do not quite get why. Could you explain this point
> again please?
>
> I do get your two points about the Clone(std::unique_ptr<T>&)
> constructor though. If I write
>
> Clone(std::unique_ptr<T> p);
>
> and passing a std::unique_ptr<T> to it the compiler complains that I am
> using a deleted function which I understand is because unique_ptr can
> not be copied, i.e. example 1 would be illegal. Therefore one would then
> have to change the client code passing std::move(p) instead of p
> directly, as in example 2. E.g. line 3202 in test-suite/marketmodel.cpp
> would
> then change as follows
>
>                          PathwiseVegasAccountingEngine
> accountingengine(boost::shared_ptr<LogNormalFwdRateEuler>(new
> LogNormalFwdRateEuler(evolver)), // method relies heavily on LMM Euler
> -                            productToUse,
> +                            std::move(productToUse),
>                              marketModel, // we need pseudo-roots and
> displacements
>                              vegaBumps,
>                              initialNumeraireValue);
>
> Example 3 would also work.
>
> Is that what you propose ?
>
> regards
>  Peter
>
> Mike Sharpe <ma...@gm...> writes:
>
> > Hi Peter,
> >
> > First off, thanks for taking a look at getting C++11 into a major
> project!
> >
> > The line
> >     Clone(std::unique_ptr<T>& p);
> >
> > looks suspicious to me. It doesn't make it clear from the client side
> that the
> > pointer ownership was changed. Compare the following three examples:
> >     // example 1
> >     std::unique_ptr<T> ptr(new T());
> >     Clone<T> newObject(ptr);
> >     // example 2
> >     std::unique_ptr<T> ptr(new T());
> >     Clone<T> newObject(std::move(ptr));
> >     //example 3
> >     Clone<T> newObject(std::unique_ptr<T>(new T()); // 3
> >
> > Example 2 makes it obvious that the constructor will retain ownership of
> the
> > unique_ptr, which isn't obvious in example 1 even though the current
> > implementation means example 1 and example 2 are the same. In addition,
> the
> > current code will remove the ability for clients to use code like
> example 3
> > completely, as you can't bind a temporary object to a non-const lvalue
> > reference. It's possible clients could run into problems even if they
> don't
> > upgrade from auto_ptr to unique_ptr if they have code like this.
> >
> > I propose that, instead of changing the function definition to
> incorporate
> > either auto_ptr or unique_ptr, that another Clone constructor is added,
> so that
> > there will be two constructors:
> >
> >   Clone(std::auto_ptr<T> ptr);
> > #ifdef QL_UNIQUE_PTR_ENABLED
> >   Clone(std::unique_ptr<T> ptr);
> > #endif
> >
> > The implementation of the auto_ptr constructor will need to make sure to
> > initialize the unique_ptr calling uniquePtr(std::move(autoPtr)) in the
> > initializer class if the internal class uses unique_ptr. This is
> > because unique_ptr has a constructor that accepts auto_ptr by rvalue
> reference
> > only, and rvalue references cannot bind to lvalues. But, you can base the
> > private data stored in clone off object internals simply off the
> > QL_UNIQUE_PTR_ENABLED #define, without clients having to change their
> code.
> > The decision to remove the auto_ptr constructor can be made later.
> >
> > Hopefully this helps. Let me know if an updated Clone.hpp would make
> things a
> > little bit clearer and I can put something together when I have some
> time.
> >
> > Mike
> >
> >
> > On Thu, May 30, 2013 at 9:13 AM, Peter Caspers <pca...@gm...>
> wrote:
> >
> >     following up the topic below: I missed to look at the test-suite,
> where
> >     some more
> >     auto_ptr would have to be replaced by unique_ptr.
> >
> >     https://github.com/pcaspers/quantlib/commit/
> >     466fbfbc423312eb7c4536ab59d861689cde70a0
> >
> >     Furthermore the header ql/utilities/clone.hpp has to be changed.
> >
> >     https://github.com/pcaspers/quantlib/commit/
> >     fe5a87dcfb60c10f1d8600b1511037e200915e4c
> >     https://github.com/pcaspers/quantlib/commit/
> >     7a12fc4c62b44d175dc794b487db0582803ef6a5
> >
> >     The test-suite now compiles and runs using g++ (with and without
> -std=
> >     c++0x)
> >     which is the main thing I guess. The whole thing does not look very
> >     nice though. Also I am not c++ expert enough to say that the upgrade
> of
> >     clone.hpp to unique_ptr should be done like this. Luigi ?
> >
> >     Thank you
> >      Peter
> >
> >     Peter Caspers <pca...@gm...> writes:
> >
> >     > Hi Luigi, all,
> >     >
> >     > we discussed compiling the core lib under the c++11 standard (e.g.
> with
> >     > g++ -std=c++0x). This produces some errors which can be fixed
> easily
> >     > (keeping backward compatibility):
> >     >
> >     > https://github.com/pcaspers/quantlib/commit/
> >     5b32b7705530264551e0622d0a2673813067be5b
> >     >
> >     > What is left is a bunch of warnings saying that std::auto_ptr is
> >     > depricated under c++11, the replacement being std::unique_ptr. A
> >     > possible solution is to replace the auto_ptr in the source by
> unique_ptr
> >     > (thus upgrading the code to c++11 already) and replace the
> unique_ptr by
> >     > auto_ptr again in case that compilation is done under c++ versions
> <
> >     > 11. I have done this here
> >     >
> >     > https://github.com/pcaspers/quantlib/commit/
> >     3e47a82a936112f12e3bb502292833d860f5aac9
> >     >
> >     > with a #define in qldefines.hpp resetting the pointers to auto_ptr
> again
> >     > based on the boost macro BOOST_NO_CXX_SMART_PTR. Unfortunately this
> >     > macro is available only in later boost versions so if one uses an
> older
> >     > one and provides no c++11 support he or she will get errors.
> Therefore
> >     > the following solution might be better
> >     >
> >     > https://github.com/pcaspers/quantlib/commit/
> >     6db965375b02094a7ea0da4e48efd7e0a1b8fd85
> >     >
> >     > using the __cplusplus macro to identify c++11. However since gcc
> sets
> >     > this macro simply to 1 in versions 4.6 (should be solved starting
> in
> >     > 4.7), I had to add another criterion for gcc based on
> >     > __GXX_EXPERIMENTAL_CXX0X_.
> >     >
> >     > For msvc 2010 the __cplusplus is also not 201103L but still
> 199711L. This
> >     > seems also to be the case in 2012 and this specific case was
> reported as
> >     a bug to
> >     > Microsoft,
> >     >
> >     > http://connect.microsoft.com/VisualStudio/feedback/details/763051/
> >     a-value-of-predefined-macro-cplusplus-is-still-199711l
> >     >
> >     > though not solved yet. Therefore I added a direct test on msvc
> 2010 or
> >     > higher versions based on _MSC_VER
> >     >
> >     > https://github.com/pcaspers/quantlib/commit/
> >     796beb89e2a707f951b77869bf869694e67ac769
> >     >
> >     > This final solution should work for all versions of msvc and gcc.
> I can
> >     not check other
> >     > compilers however.
> >     >
> >     > Since the #define seems a bit like a dirty hack we also thought of
> a
> >     > typedef. But then we would need a template typedef which is
> available
> >     only in
> >     > c++11 again and a metaprogramming-like workaround a la
> >     >
> >     > template<class T> struct PTR {
> >     >     typedef std::unique_ptr<T> Type;
> >     > };
> >     >
> >     > PTR<double>::Type a(new double(0.0));
> >     >
> >     > does not seem to improve the code either.
> >     >
> >     > Do you think we should do the upgrade to c++11 like propsed above
> or is
> >     there
> >     > maybe a better solution (I bet there is...). Or should we defer the
> >     > upgrade until a later release ? I personally would very much like
> to be
> >     > able to compile under c++11 without warnings already now.
> >     >
> >     > Thank you
> >     >  Peter
> >     >
> >     >
> >     >
> >     >
> >
> >
> ------------------------------------------------------------------------------
> >     Introducing AppDynamics Lite, a free troubleshooting tool for
> Java/.NET
> >     Get 100% visibility into your production application - at no cost.
> >     Code-level diagnostics for performance bottlenecks with <2% overhead
> >     Download for free and get started troubleshooting in minutes.
> >     http://p.sf.net/sfu/appdyn_d2d_ap1
> >     _______________________________________________
> >     QuantLib-dev mailing list
> >     Qua...@li...
> >     https://lists.sourceforge.net/lists/listinfo/quantlib-dev
>