This patch is a WIP to support P0414R2.
Details
Details
- Reviewers
mclow.lists ldionne EricWF - Group Reviewers
Restricted Project - Commits
- rGce195fb22b54: [libcxx] Re-commit: shared_ptr changes from library fundamentals (P0414R2).
rGe8c13c182a56: [libcxx] shared_ptr changes from library fundamentals (P0414R2).
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer.pass.cpp | ||
|---|---|---|
| 42 ↗ | (On Diff #201558) | I am not so sure about shared_ptr<void>. The standard says "The expression delete[] p, when T is an array type, or delete p, when T is not an array type, shall have well-defined behavior, and shall not throw exceptions." Meaning that delete T must be well defined. But the standard also says delete void is not well defined: "This implies that an object cannot be deleted using a pointer of type void* because there are no objects of type void." Therefore, I think shared_ptr<void> goes against the standard. Either way, for the overload tested here, we will have to pick between creating a deleter for type T and type Y. The former works for arrays and the latter works for void. |
| include/memory | ||
|---|---|---|
| 3741 ↗ | (On Diff #201558) | Use add_lvalue_reference. |
| test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer.pass.cpp | ||
|---|---|---|
| 42 ↗ | (On Diff #201558) | The standard says delete p must be well defined. Meaning delete Y must be well defined. Not delete T. My bad. |
| include/memory | ||
|---|---|---|
| 4028 ↗ | (On Diff #203479) | I don't love this "hack", but I am not sure there is a better way to fix the issue I describe below. Suggestions are more than welcome. |
Comment Actions
Ping. This patch now has no references to any other patches so it can now be merged immediately.
Comment Actions
Pinging ~weekly is about the best we've got. Folks might be a bit less responsive during the current global pandemic, etc.
Comment Actions
Thanks for your patience, I'm finally getting to this. This mostly LGTM, with a question:
For get and operator[], the paper says:
Remarks: When T is [not] an array type, it is unspecified whether this member function is declared. If it is declared, it is unspecified what its return type is, except that the declaration (although not necessarily the definition) of the function shall be well formed.
I think we might want to add a static_assert in the body of those functions to check that _T is/is not an array type. WDYT?
| libcxx/www/cxx1z_status.html | ||
|---|---|---|
| 131 | You can mark this as being in LLVM 11.0. | |
Comment Actions
Thanks for the review :)
@ldionne I think that's only the case for operator[], not get(). In fact, in the paper, [[ https://eel.is/c++draft/smartptr#util.smartptr.shared.obs-9 | get() is used to implement operator[]. ]]
Comment Actions
You are entirely right, I got confused by the code highlighting in the paper. Instead, it was:
T* operator->() const noexcept;
Specifically, this bullet: http://wg21.link/util.smartptr.shared.obs#7.
If it makes sense to add a static_assert in operator[], I think it makes sense to add the reverse static_assert in operator->.
Comment Actions
using static_assert breaks the ability to include <memory> in a precompiled header and then explicitly define a shared pointer type for export. You should have used enable_if on the return type instead. you can download my Xcode 12 project files from southwell.org or create your own based on the following three code files. test.cpp will fail to compile because it attempts to instantiate the [] lookup operator for export. I don't think this is a bug. It needs to do this because it doesn't know which member functions will be called when the exported type is used by clients of the library. Though this does appear to work if memory is not first included in a precompiled header so I could be misinterpreting that.
PrefixHeader.pch
#include <memory>
testtemplate.hpp
#ifndef testtemplate_
#define testtemplate_
#include <memory>
template class __attribute__ ((visibility ("default"))) std::shared_ptr<int>;
typedef std::shared_ptr<int> shared_int;
__attribute__ ((visibility ("default"))) shared_int get_int();
#endiftesttemplate.cpp
#include "testtemplate.hpp"
#include <cstdlib>
shared_int get_int()
{
return std::make_shared<int>(std::rand());
}| libcxx/include/memory | ||
|---|---|---|
| 3912–3913 | @cedral wrote:
On the one hand, I confirm your understanding of how template class std::shared_ptr<int[]> is treated by the compiler. https://godbolt.org/z/7bbasY83G On the third hand, the language-lawyer answer is that your whole program is "ill-formed, no diagnostic required," and so libc++ isn't required to make it work anyway. Users aren't allowed to explicitly instantiate library types like that. One reason is that you don't know the name of the type — is it really class std::shared_ptr, or is it struct std::shared_ptr, or is shared_ptr just an alias for __shared_ptr, etc. etc. The more important reason is exactly what you already found out: When you explicitly instantiate a class template, you get all its members, including the private ones that you couldn't otherwise mess with, and which might well not be instantiable for your particular type. Example: https://godbolt.org/z/3hb7Wb3Te | |
@cedral wrote:
On the one hand, I confirm your understanding of how template class std::shared_ptr<int[]> is treated by the compiler. https://godbolt.org/z/7bbasY83G
On the other hand, I cannot reproduce your issue with libc++ itself; the reason is that _LIBCPP_INLINE_VISIBILITY expands to a series of attributes including __attribute__((__exclude_from_explicit_instantiation__)). If you're seeing the problem only with PCHes, I wonder if there's a bad interaction between PCHes and that attribute.
Alternatively, are you using GCC (or anything else non-Clang)? It seems like GCC doesn't support that attribute, so I think on GCC you're just out of luck by design.
On the third hand, the language-lawyer answer is that your whole program is "ill-formed, no diagnostic required," and so libc++ isn't required to make it work anyway. Users aren't allowed to explicitly instantiate library types like that. One reason is that you don't know the name of the type — is it really class std::shared_ptr, or is it struct std::shared_ptr, or is shared_ptr just an alias for __shared_ptr, etc. etc. The more important reason is exactly what you already found out: When you explicitly instantiate a class template, you get all its members, including the private ones that you couldn't otherwise mess with, and which might well not be instantiable for your particular type. Example: https://godbolt.org/z/3hb7Wb3Te
Here's a real-world example from libstdc++: template class std::atomic<void*>; is a hard error. https://godbolt.org/z/eTq9TWqnK