Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Krzysztof Czainski (1czajnik_at_[hidden])
Date: 2011-05-26 17:30:33

2011/5/26 Sergiu Dotenco <sergiu.dotenco_at_[hidden]>

> Am 26.05.2011 21:23, schrieb Artyom Beilis:
> > ----- Original Message ----
> >> From: Sergiu Dotenco <sergiu.dotenco_at_[hidden]>
> >>
> >> Am 26.05.2011 18:28, schrieb Artyom Beilis:
> >>>> From: Sergiu  Dotenco <sergiu.dotenco_at_[hidden]>
> >>>>
> >>>>  Am 26.05.2011 11:22, schrieb Artyom Beilis:
> >>>>> The only advantage  I can see  in boost.pimpl over scoped_ptr or
> auto_ptr
> >>>>> is  that it does not require  explicit  destructor.
> >>>>>
> >>>>>  i.e.
> >>>>>
> >>>>>
> >>>>>     class Foo :  boost::noncopyable {
> >>>>>     public:
> >>>>>           Foo();
> >>>>>            int x() const;
> >>>>>           void  x(int v);
> >>>>>    private:
> >>>>>            struct data;
> >>>>>            std::auto_ptr<data> d;
> >>>>>     };
> >>>>>
> >>>>> No good.  You need to  add
> >>>>>
> >>>>>    ~Foo();
> >>>>>
> >>>>> So it would  know to destroy  Foo::Data correctly.
> >>>>
> >>>> You can't use std::auto_ptr  for  pimpl implementation in portable
> code.
> >>>> Doing so is  undefined behavior. To  quote ISO/IEC 14882:2003
> (§17.4.3.6,
> >>>> p.  329):
> >>>>
> >>>> "In particular, the  effects are undefined in  the following cases:
> >>>> [...]
> >>>> — if an incomplete  type  (3.9) is used as a template argument when
> >>>>   instantiating  a  template component, unless specifically allowed
> >>>>   for  that  component."
> >>>>
> >>>> which applies to most standard  library  types.
> >>>
> >>>
> >>> When you actually implement  Foo::~Foo() {} in cpp file
> >>> the type of the data object is complete so  the destructor
> >>> is installed correctly.
> >>
> >> That's not correct.  std::auto_ptr<data> is an instantiation which uses
> >> an incomplete type  as template argument. Providing an non-inline
> >> destructor doesn't magically  make the data struct used in your example
> a
> >> complete type.
> >>
> >
> > The quote you provide is too general about the library requirements.
>
> That doesn't make the quoted requirement less valid.
>
> > Particularly, looking into auto_ptr. (20.4.5.1 C++/2003 standard)
> >
> > ~auto_ptr() throw();
> > Requires: The expression delete get() is well formed.
> > Effects: delete get().
> >
> > The expression delete get() is well formed when ~auto_ptr() is called
> > in the destructor of Foo::~Foo() defined in cpp file.
> >
> > So it fills auto_ptr requirements.
>
> I don't see how is that relevant.
>
> The non-inline destructor you referred to previously is needed for
> correct object deallocation (ISO/IEC 14882:2003, §5.3.5, p. 83, par. 5).
> Obviously, templates are not instantiated in the destructor, are they?
> The std::auto_ptr<> template used in your example is instantiated right
> below the struct data; line at which point data is still an incomplete
> type. Hence the effects are undefined and the code is not portable.
>
> > No problems I can see also I'm not aware of any compiler that
> > does not handle this correctly (so far Intel, GCC, MSVC, SunStudio, HP's
> ACC,
> > DECXX)
>
> Well, that's not the point. Consider what happens if a library vendor
> decides to perform a (fully legitimate) check, as in
>
> template<class T> class auto_ptr
> {
>  // ...
>  static_assert(__is_complete_type(T), "incomplete type used");
> };
>
> If this happens, your pimpl version will no longer work.

Looks like auto_ptr really isn't suitable for pimpl. And auto_ptr has many
more draw-backs, so it is deprecated.

But getting back to the mini-review, suppose we use scoped_ptr instead of
auto_ptr. I think Artyoms arguments still stand. And then you can't forget
to make a non-inline destructor, because scoped_ptr uses checked_delete.

Regards
Kris