boostorg / core

Boost Core Utilities
132 stars 87 forks source link

Update get_pointer to support all fancy pointers #32

Closed glenfe closed 7 years ago

glenfe commented 7 years ago

This is the alternative pull request which updates get_pointer instead of adding to_raw_pointer. The other pull request is #31 which adds to_raw_pointer to Core.

pdimov commented 7 years ago

Would like to hear @igaztanaga on this (regarding the usefulness of get_pointer for going from fancy pointer to raw pointer). I don't see get_pointer actually being used in his libraries, so maybe we already have another similar facility lurking somewhere.

I see two potential problems with this implementation; first, std::pointer_traits<P>::element_type is defined for many non-pointer-like types, so the template won't SFINAE. I'm not sure whether this matters, but it's a behavior change from what we had.

Second, operator-> is undefined when the pointer is 0 (for both unique_ptr and shared_ptr). This is arguably a defect in the standard, but even if the standard is fixed, many user-defined pointers follow this practice. (Incidentally, we're missing get_pointer tests for 0.)

glenfe commented 7 years ago

I see he also has to_raw_pointer defined similarly (as in pull request #31 which was my alternative to this one).

I see one in Boost.Move in <boost/move/detail/to_raw_pointer.hpp>. Similarly in Boost.Intrusive (and Boost.Container uses it from Boost.Intrusive).

igaztanaga commented 7 years ago

Yes, in my libraries I use to_raw_pointer-like utilities. Those are needed, for instance, when trying to get a raw pointer to call allocator_traits::construct, which requires a raw pointer, from a fancy pointer. In this case, non-null pointers are guaranteed by the caller.

glenfe commented 7 years ago

Closing, to focus on #31.

Lastique commented 7 years ago

Closing, to focus on #31.

Why?

pdimov commented 7 years ago

To avoid the issue with get_pointer supporting NULL.

Lastique commented 7 years ago

To avoid the issue with get_pointer supporting NULL.

I'm not sure I understand. Whether it is implemented in get_pointer or to_raw_pointer, we face the same problem with null smart-pointers. Whatever the solution is, it can be done with the same effect in either get_pointer or to_raw_pointer.

My preference is get_pointer because I don't like duplicating tools, and currently I can't see the difference between the two functions.

pdimov commented 7 years ago

With to_raw_pointer, we have the option of disallowing NULL pointers, as it's used to construct an object at a location that comes in the form of Allocator::pointer.

Lastique commented 7 years ago

On 05/11/17 19:28, Peter Dimov wrote:

With |to_raw_pointer|, we have the option of disallowing NULL pointers, as it's used to construct an object at a location that comes in the form of |Allocator::pointer|.

Well, the comment says "get_pointer(p) extracts a ->* capable pointer from p", so I would say we have the same option with get_pointer.

glenfe commented 7 years ago

Why?

@Lastique Peter and I were talking about this on the C++ Slack: The piece that I was missing was get_pointer supporting null pointers, while the precondition for to_raw_pointer being that the pointer is not null. The motivating use case is the argument to construct for which addressof(*p) is unsuitable when *p has no object constructed in it. The risk of appropriating get_pointer for this is what Peter explained above.

pdimov commented 7 years ago

Well, the comment says "get_pointer(p) extracts a ->* capable pointer from p", so I would say we have the same option with get_pointer.

This was the original intent of get_pointer - it was a helper function used by mem_fn - but since then it's evolved into a generic way to say p.get() and is no longer limited to non-NULLs.

pdimov commented 7 years ago

I'm still unconvinced on the unsuitability of addressof(*p). Which part of the standard makes this undefined?

Lastique commented 7 years ago

I'm still unconvinced on the unsuitability of addressof(*p). Which part of the standard makes this undefined?

[unique.ptr.single.observers]/1, [util.smartptr.shared.obs]/2, for example. I'm pretty sure dereferencing raw null pointers was ok, as long as you don't read or write to the resulting reference, but now I can't find a reference in the standard to substantiate this.

pdimov commented 7 years ago

I mean in the non-NULL, to_raw_pointer, case. Here we have a fancy pointer that isn't 0, but that points to allocated raw storage suitable for an object, but with no object there yet.