[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: libcxx/include/functional:1722-1733
     if (__f.__f_ == 0)
         __f_ = 0;
     else if ((void *)__f.__f_ == &__f.__buf_)
     {
         __f_ = __as_base(&__buf_);
         __f.__f_->__clone(__f_);
     }
----------------
Here is the similar code I've mentioned.


================
Comment at: libcxx/include/functional:1821
 {
-    if ((void *)__f_ == &__buf_)
-        __f_->destroy();
-    else if (__f_)
-        __f_->destroy_deallocate();
-    __f_ = 0;
+    function::operator=(nullptr);
     if (__f.__f_ == 0)
----------------
mclow.lists wrote:
> Couldn't this be more clearly written as `*this = nullptr;` ?
Personally I prefer to call operator explicitly but checking the code shows it isn't very common. Will change.


================
Comment at: libcxx/include/functional:1822
+    function::operator=(nullptr);
     if (__f.__f_ == 0)
         __f_ = 0;
----------------
mclow.lists wrote:
> At this point `__f_ == 0` (because we just set it to 0).
> We probably don't need to do that again.
> 
This code is the same as in `function(function&& __f)`. Do you think there is enough value to deviate from that implementation?


================
Comment at: libcxx/include/functional:1843
     __f_ = 0;
+    if ((void *)__t == &__buf_)
+        __t->destroy();
----------------
mclow.lists wrote:
> I see this dance 4x in `<functional_03>` and once here. Did you give any thought to making it a function of `__base`?  Maybe call it `__clear`?
> 
> Then line #1821 can be written as `__clear();`
> 
> 
Haven't really thought about that. Not sure it can be easily done. All `__base` classes are separate templates and don't have a common ancestor. Don't want to introduce macros as it doesn't look like libc++ style. You achieve the most consistency with current code by copy-pasting and repeating the same dance a few times.


https://reviews.llvm.org/D34331