-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge rules R.12 and R.13; explain about RAII factories. #1610
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9010,8 +9010,7 @@ Here, we ignore such cases. | |
|
||
* [R.10: Avoid `malloc()` and `free()`](#Rr-mallocfree) | ||
* [R.11: Avoid calling `new` and `delete` explicitly](#Rr-newdelete) | ||
* [R.12: Immediately give the result of an explicit resource allocation to a manager object](#Rr-immediate-alloc) | ||
* [R.13: Perform at most one explicit resource allocation in a single expression statement](#Rr-single-alloc) | ||
* [R.12: Instead of manual resource allocation, use factory functions that return RAII objects](#Rr-immediate-alloc) | ||
* [R.14: Avoid `[]` parameters, prefer `span`](#Rr-ap) | ||
* [R.15: Always overload matched allocation/deallocation pairs](#Rr-pair) | ||
|
||
|
@@ -9365,73 +9364,75 @@ If you have a naked `new`, you probably need a naked `delete` somewhere, so you | |
|
||
(Simple) Warn on any explicit use of `new` and `delete`. Suggest using `make_unique` instead. | ||
|
||
### <a name="Rr-immediate-alloc"></a>R.12: Immediately give the result of an explicit resource allocation to a manager object | ||
### <a name="Rr-immediate-alloc"></a>R.12: Instead of manual resource allocation, use factory functions that return RAII objects | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point of R.12 is exactly about calling C/POSIX/WinAPI/etc "explicit resource allocation" interfaces. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That line is a footnote on what is shaping up to be a different rule. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...that is not to say it's wrong, but perhaps it needs to be more obvious that it covers the old cases of making calls to explicit APIs as the old example showed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "That line is a footnote on what is shaping up to be a different rule." — Eh? The text I was trying to point to is new in this pull request, and belongs to rule
Nobody should ever be calling "explicit resource allocation interfaces" directly in user code (conflating their actual business logic with manual resource management); that would violate rules F.2 and F.3 and perhaps F.1. Resources should be managed by resource-management classes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a fan of collapsing of the rules, the "write a wrapper" is what I always call for. |
||
|
||
##### Reason | ||
|
||
If you don't, an exception or a return may lead to a leak. | ||
|
||
##### Example, bad | ||
##### Example | ||
|
||
void f(const string& name) | ||
void bad(const char *name) | ||
{ | ||
FILE* f = fopen(name, "r"); // open the file | ||
vector<char> buf(1024); | ||
auto _ = finally([f] { fclose(f); }); // remember to close the file | ||
FILE *f = fopen(name, "r"); | ||
std::vector<char> buf(1024); | ||
// ... | ||
} | ||
|
||
The allocation of `buf` may fail and leak the file handle. | ||
|
||
##### Example | ||
If `buf`'s constructor throws, then `FILE *f` will not be `fclose`d — a resource leak. | ||
Prefer to use an RAII object such as `std::ifstream`. | ||
|
||
void f(const string& name) | ||
void good(const char *name) | ||
{ | ||
ifstream f{name}; // open the file | ||
vector<char> buf(1024); | ||
std::ifstream myfile(name); | ||
std::vector<char> buf(1024); | ||
// ... | ||
} | ||
|
||
The use of the file handle (in `ifstream`) is simple, efficient, and safe. | ||
|
||
##### Enforcement | ||
|
||
* Flag explicit allocations used to initialize pointers (problem: how many direct resource allocations can we recognize?) | ||
|
||
### <a name="Rr-single-alloc"></a>R.13: Perform at most one explicit resource allocation in a single expression statement | ||
|
||
##### Reason | ||
|
||
If you perform two explicit resource allocations in one statement, you could leak resources because the order of evaluation of many subexpressions, including function arguments, is unspecified. | ||
|
||
##### Example | ||
|
||
void fun(shared_ptr<Widget> sp1, shared_ptr<Widget> sp2); | ||
|
||
This `fun` can be called like this: | ||
|
||
// BAD: potential leak | ||
fun(shared_ptr<Widget>(new Widget(a, b)), shared_ptr<Widget>(new Widget(c, d))); | ||
void bad() { | ||
return func( | ||
std::shared_ptr<Widget>(new Widget(a, b)), | ||
std::shared_ptr<Gadget>(new Gadget(c, d)) | ||
); | ||
} | ||
|
||
This is exception-unsafe because the compiler may reorder the two expressions building the function's two arguments. | ||
In particular, the compiler can interleave execution of the two expressions: | ||
Memory allocation (by calling `operator new`) could be done first for both objects, followed by attempts to call the two `Widget` constructors. | ||
If one of the constructor calls throws an exception, then the other object's memory will never be released! | ||
Prior to C++17, it was possible for this code to evaluate `new Widget(a, b)`, | ||
followed by `new Gadget(c, d)`, followed by the constructors of both `shared_ptr`s. | ||
If either `new Gadget` or the constructor of the first `shared_ptr` were to throw | ||
an exception, an allocation would be leaked. | ||
|
||
This subtle problem has a simple solution: Never perform more than one explicit resource allocation in a single expression statement. | ||
For example: | ||
`shared_ptr<Widget>` is an RAII type, but `Widget*` is not; therefore we should | ||
not be making a direct call to `new`, which returns `Widget*`. The best solution | ||
is to use the factory function `std::make_shared`, which returns an RAII object | ||
without ever exposing the non-RAII type `Widget*` to our code. | ||
|
||
shared_ptr<Widget> sp1(new Widget(a, b)); // Better, but messy | ||
fun(sp1, new Widget(c, d)); | ||
void good() { | ||
return func( | ||
std::make_shared<Widget>(a, b), | ||
std::make_shared<Gadget>(c, d) | ||
); | ||
} | ||
|
||
The best solution is to avoid explicit allocation entirely use factory functions that return owning objects: | ||
If there is no factory wrapper for your RAII type yet, write one! | ||
|
||
fun(make_shared<Widget>(a, b), make_shared<Widget>(c, d)); // Best | ||
void bad(const char *name) | ||
{ | ||
FILE *f = fopen(name, "r"); // bad | ||
AutoFclosingFile myfile(f); | ||
// ... | ||
} | ||
|
||
Write your own factory wrapper if there is not one already. | ||
void good(const char *name) | ||
{ | ||
AutoFclosingFile myfile = my::open_file(name, "r"); | ||
// ... | ||
} | ||
|
||
##### Enforcement | ||
|
||
* Flag explicit allocations used to initialize pointers (problem: how many direct resource allocations can we recognize?) | ||
* Flag expressions with multiple explicit resource allocations (problem: how many direct resource allocations can we recognize?) | ||
|
||
### <a name="Rr-ap"></a>R.14: Avoid `[]` parameters, prefer `span` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be assuming that you're the author of the factory function in question? What if it's an old third party library? or it's a c library? Or the person next to you wrote it and you don't want to have to go and change all the places it's already called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's covered on line https://github.com/isocpp/CppCoreGuidelines/pull/1610/files#diff-feb71ecadc563b52e66838adbd6b8e30R9418