CppCoreGuidelines icon indicating copy to clipboard operation
CppCoreGuidelines copied to clipboard

Possible exception to R.11, placement syntax

Open daviddetweiler opened this issue 5 years ago • 16 comments

Custom allocation is common enough in some applications that placement new should be permitted. This is especially useful when doing binary serialization of data structures and placement new is necessary to prevent pointer aliasing UB. Since the CP section does explicitly mention "if an expert, go ahead and use lock-free," R.11 should have a similar exception for placement syntax.

C++17 has destroy_at() to facilitate this, but no good equivalent for in-place construction.

Perhaps more generally, allowing non-allocating new syntaxes that are similar in function to placement new (i.e. logging).

daviddetweiler avatar May 05 '19 04:05 daviddetweiler

This is especially common in embedded systems, where we will pre-allocate memory pools and then construct objects from pool memory using placement new.

phillipjohnston avatar May 05 '19 12:05 phillipjohnston

C++17 has destroy_at() to facilitate this, but no good equivalent for in-place construction.

C++20 is adding uninitialized_construct_using_allocator as an alternative to a "naked placement new", but it doesn't solve the resource management problem. With any use of placement new you're still responsible for managing (probably separately) the lifetime of the object and the lifetime of the storage.

I'm in two minds whether an exception for placement new is a good idea. On the one hand, it doesn't make it any easier to write correct code (as you have two separate lifetimes to manually manage) so just saying it's fine to do that doesn't really help anybody. But on the other hand if you're doing that you're already in "experts only" territory and "consider using make_unique instead" is probably not a suitable alternative. The problem with the latter view is that plenty of non-experts have to do it (not all embedded programmers are automatically expert programmers), and so better guidelines and better utilities to help them would still be valuable.

Now that I think of it, might the std::pmr model be more appropriate for custom allocators?

In what way? Do you mean the scoped allocator model, which pmr::polymorphic_allocator also uses, or the "all allocation goes through virtual function and has more space and time overhead" part of the std::pmr model? :-)

jwakely avatar May 06 '19 12:05 jwakely

This is especially common in embedded systems, where we will pre-allocate memory pools and then construct objects from pool memory using placement new.

That means you don't use make_unique, but doesn't mean you can't use unique_ptr with a custom deleter that only destroys the object, without deallocating the storage.

For cases where the objects are never destroyed, leaking them isn't a concern anyway, because "failing to run the destructor" is a feature, not a bug. In those cases a warning from a checker tool isn't helpful. I haven't thought this through properly, but I wonder if using gsl::owner<void*> as the placement new operand would make sense as a way to tell guidelines checkers you know what you're doing:

X* p = new (gsl::owner<void*>{mem}) X{args};

Semantically this just invokes the usual operator new(size_t, void) for placement new, but is lexically distinct and checkers could be taught about it,.

jwakely avatar May 06 '19 12:05 jwakely

That means you don't use make_unique, but doesn't mean you can't use unique_ptr with a custom deleter that only destroys the object, without deallocating the storage.

... don't I need to use placement new inside of that unique_ptr if I'm using pre-allocated memory?

I'm not arguing that lifetimes need to be manually managed. I'm pro-unique_ptr.

phillipjohnston avatar May 06 '19 13:05 phillipjohnston

R. 11 as it is leaves a semantic hole. Ideally, we'd just use unique_ptr(new (buffer) type{});, but new would still be flagged under R.11. There is no alternative syntax for in-place construction. Safety could potentially be enforced by leaving R. 11 unchanged and adding a gsl::construct() similar to make_unique that just constructs in place and hands back a unique_ptr with a destroy_at delete. That completely patches the hole AFAIK. Even standard allocator, which have non-owning construction, can immediately release() the pointer to the caller.

Perhaps it wasn't made clear, but the reason this is a concern in the first place is that allocator designs that separate memory allocation and object construction require placement syntax to even be expressible in C++.

Although at that point the suggested construct() is just renaming placement new.

daviddetweiler avatar May 06 '19 14:05 daviddetweiler

@phillipjohnston what do you mean "inside of that unique_ptr"? There's no memory allocation inside unique_ptr. It is constructed with a pointer, and it passes that pointer to a deleter when it is destroyed or reset. Any allocation or deallocation happens before construction (to obtain the pointer) or inside the deleter.

jwakely avatar May 06 '19 14:05 jwakely

Per @daviddetweiler, this is the usage that I mean (I know that the deleter is missing but must be specified, this is just to illustrate the use of placement new to generate the pointer that unique_ptr will manage):

unique_ptr(new (buffer) type{});

Unless there is an alternative syntax that provides equivalent functionality with unique_ptr without a placement new, I don't think the conversation should continue to focus on unique_ptr.

phillipjohnston avatar May 06 '19 14:05 phillipjohnston

Ideally, we'd just use unique_ptr(new (buffer) type{});,

That would use delete and try to deallocate your buffer. As I said, you need a custom deleter.

but new would still be flagged under R.11. There is no alternative syntax for in-place construction.

I just suggested one above.

Safety could potentially be enforced by leaving R. 11 unchanged and adding a gsl::construct() similar to make_unique that just constructs in place and hands back a unique_ptr with a destroy_at delete.

N.B. You don't want it to use destroy_at directly because that would increase the size of the unique_ptr, you want an empty class type as the deleter.

Perhaps it wasn't made clear, but the reason this is a concern in the first place is that allocator designs that separate memory allocation and object construction require placement syntax to even be expressible in C++.

I assure you I know how allocators work in C++ :-)

I don't see how the std::pmr model is relevant. You can already avoid using placement new for conventional allocators, because a custom allocator doesn't need to provide a construct member, as allocator_traits::construct provides a sensible default, and so the placement new is done for you by the standard library implementation. That's been the case since 2011.

So if your concern is just for implementing custom allocators, there's no issue. Stop writing your own construct member and use the allocator_traits::construct default.

In any case, the guidelines are not really aimed at people implementing custom allocators and custom containers. It's not practical to implement something like std::vector or std::map entirely within the guidelines, precisely because you need to manage memory and object lifetimes manually.

jwakely avatar May 06 '19 14:05 jwakely

He asked "Where is this documented?" and I think it's been said a few times in the issue discussions here, e.g. https://github.com/isocpp/CppCoreGuidelines/issues/862#issuecomment-286203744

the Guidelines are designed to apply to the majority of user code. People who implement libraries, or have sections of very low-level code (e.g. OS kernels or hardware access in device drivers) may find that some specific guidelines do not apply to them. That doesn't limit the usefulness of the Guidelines as most people use a vector or graph data structure, fewer people implement one.

Also the introduction to the guidelines says:

These guidelines address the core of C++ and its use. We expect that most large organizations, specific application areas, and even large projects will need further rules, possibly further restrictions, and further library support. For example, hard-real-time programmers typically can't use free store (dynamic memory) freely and will be restricted in their choice of libraries. We encourage the development of such more specific rules as addenda to these core guidelines. Build your ideal small foundation library and use that, rather than lowering your level of programming to glorified assembly code.

The question comes down to if custom containers and allocation are within the scope of the guidelines

They are still within the scope, but the people implementing those are not who the guidelines are primarily aimed at. Not every guideline is applicable to every piece of code, and they're guidelines not inviolable rules. If a guideline doesn't make sense in a particular context, maybe it's not necessary to follow the guideline (and maybe the guideline should call that out as an exception, but it's not practical or IMHO helpful to try and list every possible exception to every guideline).

I'm still curious why allocator_traits::construct isn't usable as a replacement for using placement new with custom allocators and in custom containers. Yes, it's just renaming placement new, but it will still avoid warnings from checkers that don't like any naked new.

jwakely avatar May 06 '19 19:05 jwakely

Ok, I get that. std::allocator_traits::construct() just didn't seem all that clean considering it ties placement syntax to the standard allocator model, and it seems odd to have to call the standard allocator on memory that wasn't allocated by the standard allocator (such as stack vectors). But I guess that is getting far enough into exceptional territory at that point.

daviddetweiler avatar May 06 '19 20:05 daviddetweiler

I'm not sure what you mean by "the standard allocator", but if you mean std::allocator, that's not what I'm talking about. std::allocator_traits is not std::allocator. But I think we're talking past each other now, I think I'm misunderstanding what you mean by "the standard allocator model" and "the std::pmr model".

I'm not trying to nitpick (as the deleted comment said), I'm trying to be precise to actually understand the problem.

jwakely avatar May 06 '19 20:05 jwakely

I deleted my comment for a reason. It was antagonistic and did not further the conversation. You guys can stop referencing it at any time :)

phillipjohnston avatar May 06 '19 20:05 phillipjohnston

Well, if we mean std::allocator_traits::construct(), suppose I wanted to make a logging implementation of it for a custom allocator. If I still wanted to use the default for the actual construction (instead of placement new), I'd have to use something like allocator_traits<std::allocator>::construct() inside my own construct(), and that's just odd. Unless I'm forgetting a way to reference the default from inside a specialization (probable)?

daviddetweiler avatar May 06 '19 20:05 daviddetweiler

Editors call: We agree that placement new isn't really "new", it's C++'s idiomatic quirky way of spelling "invoke the constructor." So we should add an exception to this item since it's not really an allocation function.

hsutter avatar May 09 '19 18:05 hsutter

The exception should however still say that it's for low-level manual lifetime management and should have a corresponding destroy_at or explicit destructor call, and typically should be hidden away inside a library component (e.g., a container) that encapsulates it.

hsutter avatar May 09 '19 18:05 hsutter

I use placement new to create messages that are allocated by a specific allocate platform function which has his corresponding deallocate function.

auto* msg pltf_allocate(Message_id, sizeof(Message)); auto ptr = new (pltf_payload(msg)) Message{1,2};

I could mask this using a construct_at function

auto ptr = construct_at<Message>(pltf_payload(msg), 1,2);

The advantage of the placement new form respect to a construct_at fuction is that I can use C++20 designated initializers and continue to avoid implicit conversions.

new (pltf_payload(msg)) Message {.a=1, .b=2};

viboes avatar Sep 15 '19 07:09 viboes