This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Value-initialize unique_ptr's deleter_type
ClosedPublic

Authored by bergjohan on Nov 10 2021, 2:10 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG68e2231f8724: [libc++] Value-initialize unique_ptr's deleter_type
Summary

According to the C++ standard, the stored pointer and the stored deleter
should be value-initialized.

Diff Detail

Event Timeline

bergjohan requested review of this revision.Nov 10 2021, 2:10 PM
bergjohan created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 2:10 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Nov 18 2021, 9:50 AM
ldionne added a subscriber: ldionne.

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

libcxx/include/__memory/unique_ptr.h
187

We have the same problem here AFAICT.

411

And here!

This revision now requires changes to proceed.Nov 18 2021, 9:50 AM
bergjohan updated this revision to Diff 388313.Nov 18 2021, 1:47 PM

Add missing value-initializations

bergjohan added a comment.EditedNov 18 2021, 1:53 PM

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

Thanks for the review! I just noticed the diff from the spec.

I can't believe I missed those two cases. I made the changes and added another test case.

I also noticed there's a missing value initialization in the constructor taking an auto_ptr. But I was not able to run the auto_pointer.pass.cpp test because it was marked as unsupported.
Should we update that constructor as well, and if so, how do I run the test?

Wait, scratch that, the auto_ptr constructor should be disable with a non-default deleter, so I guess it doesn't really make any difference.

bergjohan marked 2 inline comments as done.Nov 18 2021, 1:54 PM
bergjohan edited the summary of this revision. (Show Details)Nov 19 2021, 4:17 AM

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

Thanks for the review! I just noticed the diff from the spec.

I can't believe I missed those two cases. I made the changes and added another test case.

I also noticed there's a missing value initialization in the constructor taking an auto_ptr. But I was not able to run the auto_pointer.pass.cpp test because it was marked as unsupported.
Should we update that constructor as well, and if so, how do I run the test?

It looks like the auto_ptr test requires C++11 or C++14. You can see how to run the test suite with different parameters here: https://libcxx.llvm.org/TestingLibcxx.html#usage

Long story short, you can do:

<build-dir>/bin/llvm-lit -sv libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/auto_pointer.pass.cpp --param std=c++14
bergjohan updated this revision to Diff 388542.Nov 19 2021, 9:48 AM

Value-initialize in auto_ptr constructor

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

Thanks for the review! I just noticed the diff from the spec.

I can't believe I missed those two cases. I made the changes and added another test case.

I also noticed there's a missing value initialization in the constructor taking an auto_ptr. But I was not able to run the auto_pointer.pass.cpp test because it was marked as unsupported.
Should we update that constructor as well, and if so, how do I run the test?

It looks like the auto_ptr test requires C++11 or C++14. You can see how to run the test suite with different parameters here: https://libcxx.llvm.org/TestingLibcxx.html#usage

Long story short, you can do:

<build-dir>/bin/llvm-lit -sv libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/auto_pointer.pass.cpp --param std=c++14

Thanks, that did the trick!

I've added value-initialization to the auto_ptr constructor to match the spec, however, I'm not able to add a test for this specific case since this constructor is disabled for non-default deleters. The current tests for the auto_ptr constructor are still passing after the change.

ldionne accepted this revision.Nov 22 2021, 10:19 AM

Can you please rebase onto main? That will re-trigger CI and it should pass.

Also, if you want me to commit this for you, please provide Author Name <email@domain>. Thanks!

This revision is now accepted and ready to land.Nov 22 2021, 10:19 AM
bergjohan updated this revision to Diff 389030.Nov 22 2021, 1:33 PM

Rebase onto main

Can you please rebase onto main? That will re-trigger CI and it should pass.

Also, if you want me to commit this for you, please provide Author Name <email@domain>. Thanks!

Done!

Thanks, you can use "Johan Berg <47738833+bergjohan@users.noreply.github.com>

This revision was automatically updated to reflect the committed changes.