vsapsai added a comment.
It is agreed that it is untenable to support reentrancy for `std::function`
assignment operators and I'm not trying to achieve that. Here we restrict
reentrancy only to `std::function::operator=(nullptr_t)`.
@mclow.lists where should the tests go if the standard specifi
BillyONeal added subscribers: STL_MSFT, BillyONeal.
BillyONeal added a comment.
@mclow.lists
@STL_MSFT
Why did tests for this this go into std? [reentrancy]/1 says this isn't
required to work. Moreover, assignments in the dtor like this *can't* work in
the general case because they would try to
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX330885: [libcxx] func.wrap.func.con: Unset function before
destroying anything (authored by vsapsai, committed by ).
Ch
vsapsai updated this revision to Diff 143797.
vsapsai added a comment.
- Move tests to test/std.
https://reviews.llvm.org/D34331
Files:
libcxx/include/__functional_03
libcxx/include/functional
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_ree
mclow.lists accepted this revision.
mclow.lists added a comment.
Please move the tests into test/std and commit.
https://reviews.llvm.org/D34331
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/c
vsapsai added a comment.
Herald added a subscriber: jkorous.
Ping.
https://reviews.llvm.org/D34331
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai added a comment.
Ping.
https://reviews.llvm.org/D34331
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai added a comment.
Are there more thoughts on this change?
https://reviews.llvm.org/D34331
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai updated this revision to Diff 139506.
vsapsai added a comment.
- Replace `function::operator=(nullptr);` with `*this = nullptr;`
https://reviews.llvm.org/D34331
Files:
libcxx/include/__functional_03
libcxx/include/functional
libcxx/test/libcxx/utilities/function.objects/func.wrap
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 th
mclow.lists added a comment.
A few small comments...
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
vsapsai updated this revision to Diff 139369.
vsapsai edited the summary of this revision.
vsapsai added a comment.
- Implement the same functionality for C++98 and C++03.
- Use `DoNotOptimize` instead of `asm`.
Didn't move the tests as the standard doesn't require assignment operator to be
reent
arphaman added a comment.
Ah, I got it. There's a `__functional_03` header that seems to implement
`function` for C++03 because of manual variadic template expansions.
You have to update the operators in the header as well.
https://reviews.llvm.org/D34331
arphaman added a comment.
If you look at the AST diff between C++11 and C++03 this error still happens
because of how `nullptr` is processed:
C++11:
| |-CXXDestructorDecl 0x7fdab27a2498 line:24:3 used ~A
'void (void) noexcept'
| | `-CompoundStmt 0x7fdab27bcfb8
| | |-GCCAsmStmt 0x7fda
arphaman added a comment.
The tests actually do compile in C++03, but they still fail because of infinite
recursion:
frame #196562: 0x0001155c
nullptr_t_assign_reentrant.pass.cpp.exe`A::~A() + 92
frame #196563: 0x00011405
nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__
dexonsmith added a comment.
In https://reviews.llvm.org/D34331#831686, @arphaman wrote:
> Don't you need `// UNSUPPORTED: c++98, c++03` since `std::function` is
> supported in C++11 only?
The tests in libcxx/test/std/utilities/function.objects/ don't have UNSUPPORTED
lines, and I don't see a
arphaman added a comment.
Don't you need `// UNSUPPORTED: c++98, c++03` since `std::function` is
supported in C++11 only?
https://reviews.llvm.org/D34331
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/
dexonsmith added a comment.
In https://reviews.llvm.org/D34331#803452, @EricWF wrote:
> In https://reviews.llvm.org/D34331#803105, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/D34331#802747, @EricWF wrote:
> >
> > > @dexonsmith Mind if I hijack this and check in your changes to
> > > ``
EricWF added a comment.
In https://reviews.llvm.org/D34331#803105, @dexonsmith wrote:
> In https://reviews.llvm.org/D34331#802747, @EricWF wrote:
>
> > @dexonsmith Mind if I hijack this and check in your changes to
> > `` with my tests?
>
>
> Not at all.
>
> In https://reviews.llvm.org/D34331#80
dexonsmith added a comment.
In https://reviews.llvm.org/D34331#802747, @EricWF wrote:
> @dexonsmith Mind if I hijack this and check in your changes to ``
> with my tests?
Not at all.
In https://reviews.llvm.org/D34331#802821, @EricWF wrote:
> @dexonsmith I'm not sure it's sane to allow reent
EricWF added a comment.
> However there is another bug here. operator=(function&&) doesn't correctly
> call the destructor of the functor. I'll fix that as a separate commit.
Woops, I misread the diff. There is no existing bug W.R.T. missing destructor
calls.
https://reviews.llvm.org/D34331
EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.
@dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you
explain why you think it is? Should the copy assignment operator allow
reentrancy as well?
However there is
EricWF added a comment.
@dexonsmith Mind if I hijack this and check in your changes to ``
with my tests?
Comment at:
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp:1
+//===-
dexonsmith added a comment.
Ping!
https://reviews.llvm.org/D34331
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dexonsmith created this revision.
Be defensive against a reentrant std::function::operator=(), in case the held
function object has a non-trivial destructor. Destroying the function object
in-place can lead to the destructor being called twice.
rdar://problem/32836603
https://reviews.llvm.or
25 matches
Mail list logo
