Modify rule S1048,S3654: LaYC format
This commit is contained in:
Marco Borgeaud
GitHub
parent
149a133550
commit
c8cb1f6fb0
1
rules/S1048/cfamily/documentation.adoc
Normal file
1
rules/S1048/cfamily/documentation.adoc
Normal file
@ -0,0 +1 @@
|
||||
* {cpp} reference - https://en.cppreference.com/w/cpp/language/function-try-block[function-try-block]
|
||||
182
rules/S1048/cfamily/how_to_fix_it.adoc
Normal file
182
rules/S1048/cfamily/how_to_fix_it.adoc
Normal file
@ -0,0 +1,182 @@
|
||||
== How to fix it
|
||||
|
||||
This rule raises an issue when an exception is thrown from within a destructor or from a function transitively called by a destructor.
|
||||
|
||||
Such an exception should be caught and handled before the destructor exits.
|
||||
|
||||
=== Code examples
|
||||
|
||||
==== Noncompliant code example
|
||||
|
||||
In the following example, an exception is thrown inside the `Builder` destructor when calling `std::optional::value` on an empty optional.
|
||||
It follows that the program abruptly terminates when `b` gets destroyed during stack unwinding in `application()` if the user name lookup fails.
|
||||
|
||||
[source,cpp,diff-id=1,diff-type=noncompliant]
|
||||
----
|
||||
void logBuilderDestruction(int userId);
|
||||
|
||||
class Builder {
|
||||
std::optional<int> _id;
|
||||
|
||||
public:
|
||||
void setId(int userId) { _id = userId; }
|
||||
|
||||
~Builder() {
|
||||
auto userId = _id.value(); // Noncompliant: may throw std::bad_optional_access
|
||||
logBuilderDestruction(userId);
|
||||
}
|
||||
};
|
||||
|
||||
std::unordered_map<std::string, int>& getDatabase();
|
||||
|
||||
int lookupUserId(std::string const& name) {
|
||||
return getDatabase().at(name); // May throw std::out_of_range.
|
||||
}
|
||||
|
||||
void application(std::string const& name) {
|
||||
Builder b;
|
||||
b.setId(lookupUserId(name));
|
||||
// lookupUserId throws an exception when the name is unknown.
|
||||
// This causes the stack to unwind: local variables alive at
|
||||
// this point, such a "b", are destroyed. This happens before
|
||||
// the invocation of "b.setId()" so "b._id" is still empty
|
||||
// when its destructor is executed.
|
||||
|
||||
// ...
|
||||
}
|
||||
----
|
||||
|
||||
==== Compliant solution
|
||||
|
||||
The solution below uses `std::optional::value_or` to ensure no exceptions are thrown from the destructor.
|
||||
|
||||
[source,cpp,diff-id=1,diff-type=compliant]
|
||||
----
|
||||
void logBuilderDestruction(int userId);
|
||||
|
||||
class Builder {
|
||||
std::optional<int> _id;
|
||||
|
||||
public:
|
||||
void setId(int userId) { _id = userId; }
|
||||
|
||||
~Builder() {
|
||||
auto userId = _id.value_or(-1); // Compliant: never throws.
|
||||
logBuilderDestruction(userId);
|
||||
}
|
||||
};
|
||||
|
||||
std::unordered_map<std::string, int>& getDatabase();
|
||||
|
||||
int lookupUserId(std::string const& name) {
|
||||
return getDatabase().at(name); // May throw std::out_of_range.
|
||||
}
|
||||
|
||||
void application(std::string const& name) {
|
||||
Builder b;
|
||||
b.setId(lookupUserId(name));
|
||||
// lookupUserId throws an exception when the name is unknown.
|
||||
// This causes the stack to unwind: local variables alive at
|
||||
// this point, such a "b", are destroyed. This happens before
|
||||
// the invocation of "b.setId()" so "b._id" is still empty
|
||||
// when its destructor is executed.
|
||||
|
||||
// ...
|
||||
}
|
||||
----
|
||||
|
||||
==== Noncompliant code example
|
||||
|
||||
How to deal with exceptions in destructors highly depends on the application.
|
||||
Below, we present another way to solve the issue with an RAII-based class representing a temporary directory.
|
||||
|
||||
[source,cpp,diff-id=2,diff-type=noncompliant]
|
||||
----
|
||||
// Delete the given directory; throws OSException on failure.
|
||||
void deleteDirectory(Path path) noexcept(false) {
|
||||
// ...
|
||||
}
|
||||
|
||||
class TemporaryDirectory {
|
||||
Path tmp;
|
||||
|
||||
public:
|
||||
TemporaryDirectory(); // Create a new temporary directory.
|
||||
|
||||
TemporaryDirectory(TemporaryDirectory const&) = delete;
|
||||
TemporaryDirectory(TemporaryDirectory&&) = delete;
|
||||
TemporaryDirectory& operator=(TemporaryDirectory const&) = delete;
|
||||
TemporaryDirectory& operator=(TemporaryDirectory&&) = delete;
|
||||
|
||||
~TemporaryDirectory() {
|
||||
deleteDirectory(tmp); // Noncompliant: may throw.
|
||||
}
|
||||
};
|
||||
----
|
||||
|
||||
==== Compliant solution
|
||||
|
||||
Depending on the use case for those temporary directories, applying some remedial actions to avoid leaking secrets may be essential.
|
||||
Yet, it may be reasonable to simply log and silence the exception, for example, in the context of unit tests.
|
||||
|
||||
It is possible to redesign the class:
|
||||
|
||||
* Add a `remove` member function for scenarios that must carefully and reliably handle any `OSException`.
|
||||
In sensitive contexts, the application should not solely rely on the destructor.
|
||||
* Call this `remove` function from the destructor and catch any exception.
|
||||
This preserves the original class intent: an attempt to delete the directory is made.
|
||||
|
||||
[source,cpp,diff-id=2,diff-type=compliant]
|
||||
----
|
||||
// Delete the given directory; throws OSException on failure.
|
||||
void deleteDirectory(Path path) noexcept(false) {
|
||||
// ...
|
||||
}
|
||||
|
||||
class TemporaryDirectory {
|
||||
Path tmp;
|
||||
|
||||
public:
|
||||
TemporaryDirectory(); // Create a new temporary directory.
|
||||
|
||||
TemporaryDirectory(TemporaryDirectory const&) = delete;
|
||||
TemporaryDirectory(TemporaryDirectory&&) = delete;
|
||||
TemporaryDirectory& operator=(TemporaryDirectory const&) = delete;
|
||||
TemporaryDirectory& operator=(TemporaryDirectory&&) = delete;
|
||||
|
||||
void remove() { deleteDirectory(tmp); }
|
||||
|
||||
~TemporaryDirectory() {
|
||||
try {
|
||||
remove();
|
||||
} catch (OSException const& e) {
|
||||
logFailureToRemoveDirectory(e);
|
||||
}
|
||||
}
|
||||
};
|
||||
----
|
||||
|
||||
=== Pitfalls
|
||||
|
||||
Using a _function-try-block_ in a destructor does not prevent the destructor from exiting with an exception.
|
||||
|
||||
For example, the following destructor does not prevent the exception from escaping.
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
~TemporaryDirectory() try {
|
||||
remove();
|
||||
} catch (OSException const& e) {
|
||||
logFailureToRemoveDirectory(e);
|
||||
}
|
||||
// `e` is automatically rethrow as if `throw;` was used.
|
||||
----
|
||||
|
||||
Instead, a _try-block_ should be used within the destructor's body.
|
||||
|
||||
=== Going the extra mile
|
||||
|
||||
It is possible to detect whether a destructor is executed during stack unwinding and act accordingly; for example, to implement a transaction rollback action.
|
||||
|
||||
The {cpp}17 https://en.cppreference.com/w/cpp/error/uncaught_exception[`std::uncaught_exceptions`] function can be used for this purpose, as explained in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf[N4152].
|
||||
This function ends with an `s` and should not be confused with `std::uncaught_exception`, which got removed in {cpp}20 for the reasons exposed in the paper.
|
||||
1
rules/S1048/cfamily/related_rules.adoc
Normal file
1
rules/S1048/cfamily/related_rules.adoc
Normal file
@ -0,0 +1 @@
|
||||
* S3654 - Destructors should be "noexcept"
|
||||
@ -1,52 +1,4 @@
|
||||
== Why is this an issue?
|
||||
|
||||
When an exception is thrown, the call stack is unwound up to the point where the exception is to be handled. The destructors for all automatic objects declared between the point where the exception is thrown and where it is to be handled will be invoked. If one of these destructors exits with an exception, then the program will terminate in an implementation-defined manner, potentially yielding unexpected results.
|
||||
|
||||
|
||||
Note that it is acceptable for a destructor to throw an exception that is handled within the destructor, for example within a try-catch block.
|
||||
|
||||
|
||||
=== Noncompliant code example
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
class C1 {
|
||||
public: ~C1() {
|
||||
throw(42); // Noncompliant - destructor exits with an exception
|
||||
}
|
||||
};
|
||||
|
||||
void foo() {
|
||||
C1 c; // program terminates when c is destroyed
|
||||
throw(10);
|
||||
}
|
||||
----
|
||||
|
||||
|
||||
=== Compliant solution
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
class C1 {
|
||||
public: ~C1() {
|
||||
try {
|
||||
throw(42); // Compliant - exception will not leave destructor
|
||||
} catch (int i) { // int handler
|
||||
// Handle int exception throw by destructor
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
void foo() {
|
||||
C1 c;
|
||||
throw(10);
|
||||
}
|
||||
----
|
||||
|
||||
|
||||
== Resources
|
||||
|
||||
* MISRA {cpp}:2008, 15-5-1 - A class destructor shall not exit with an exception.
|
||||
include::../../../shared_content/cfamily/exception_in_destructor.adoc[]
|
||||
|
||||
ifdef::env-github,rspecator-view[]
|
||||
'''
|
||||
|
||||
1
rules/S3654/cfamily/documentation.adoc
Normal file
1
rules/S3654/cfamily/documentation.adoc
Normal file
@ -0,0 +1 @@
|
||||
* {cpp} reference - https://en.cppreference.com/w/cpp/language/noexcept_spec[`noexcept` specifier]
|
||||
49
rules/S3654/cfamily/how_to_fix_it.adoc
Normal file
49
rules/S3654/cfamily/how_to_fix_it.adoc
Normal file
@ -0,0 +1,49 @@
|
||||
== How to fix it
|
||||
|
||||
This rule raises an issue when a destructor is not `noexcept`.
|
||||
|
||||
Usually, nothing needs to be written in the source code because destructors are `noexcept` by default.
|
||||
However, a destructor becomes not `noexcept` when:
|
||||
|
||||
* The base class or a data member has a non `noexcept` destructor;
|
||||
* The destructor is decorated with the `noexcept(expression)` and `expression` evaluates to `false`.
|
||||
|
||||
The code should be modified to avoid those two scenarios.
|
||||
|
||||
=== Code examples
|
||||
|
||||
==== Noncompliant code example
|
||||
|
||||
// Not using diff-highlighting since most lines are changed.
|
||||
[source,cpp]
|
||||
----
|
||||
struct A {
|
||||
~A() noexcept(false) {} // Noncompliant
|
||||
};
|
||||
|
||||
struct C {
|
||||
A a; // This member data prevents automatic declaration of the destructor as noexcept
|
||||
|
||||
~C() { // Noncompliant by transitivity
|
||||
// ...
|
||||
}
|
||||
};
|
||||
----
|
||||
|
||||
|
||||
==== Compliant solution
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
struct A {
|
||||
~A() noexcept(true) {} // Compliant
|
||||
};
|
||||
|
||||
struct C {
|
||||
A a;
|
||||
|
||||
~C() { // Compliant, noexcept by default
|
||||
// ...
|
||||
}
|
||||
};
|
||||
----
|
||||
1
rules/S3654/cfamily/related_rules.adoc
Normal file
1
rules/S3654/cfamily/related_rules.adoc
Normal file
@ -0,0 +1 @@
|
||||
* S1048 - Destructors should not throw exceptions
|
||||
@ -1,75 +1,4 @@
|
||||
Throwing an exception from a destructor may result in a call to `std::terminate` and can introduce undefined behavior, meaning that your program could be terminated abruptly without being allowed to perform a clean shutdown.
|
||||
|
||||
== Why is this an issue?
|
||||
|
||||
Destructors are usually (implicitly) declared as `noexcept` by default such that `std::terminate` is called when they throw an exception.
|
||||
Destructors may still propagate an exception if they are explicitly declared as `noexcept(false)`.
|
||||
However, even a destructor declared as `noexcept(false)` will call `std::terminate` if it throws during stack unwinding.
|
||||
|
||||
A commonly used example that highlights the severity of the underlying problem is presented in what follows:
|
||||
|
||||
The destructor of a container needs to call the destructors for all managed objects.
|
||||
Suppose a call to an object's destructor throws an exception.
|
||||
In that case, there are only two _conceptual_ ways to proceed:
|
||||
|
||||
1. Abort destruction. This will result in a partially destroyed object and possibly many more objects whose destructor has not been called.
|
||||
2. Ignore the exception and proceed with destroying the remaining objects. However, this potentially results in more partially destroyed objects if further destructors throw an exception.
|
||||
|
||||
Both options are undesired; hence, whenever an object managed by a C++ standard container throws an exception, the behavior is undefined.
|
||||
|
||||
Thus, destructors should never `throw` exceptions.
|
||||
Instead, they should catch and handle those thrown by the functions they call and be `noexcept`.
|
||||
|
||||
|
||||
This rule raises an issue when a destructor is not `noexcept`. Destructors are `noexcept` by default, so usually nothing needs to be written in the source code. However, a destructor becomes not `noexcept` if:
|
||||
|
||||
* the base class or a data member has a non `noexcept` destructor,
|
||||
* the destructor is decorated with the `noexcept` keyword followed by something that evaluates to false.
|
||||
|
||||
== How to fix it
|
||||
=== Code examples
|
||||
|
||||
==== Noncompliant code example
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
struct A {
|
||||
~A() noexcept(false) {} // Noncompliant
|
||||
};
|
||||
|
||||
struct C {
|
||||
/* ... */
|
||||
A a; // This member data prevents automatic declaration of the destructor as noexcept
|
||||
~C() { // Noncompliant
|
||||
/* ... */
|
||||
}
|
||||
};
|
||||
----
|
||||
|
||||
|
||||
==== Compliant solution
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
struct A {
|
||||
~A() noexcept(true) {}
|
||||
};
|
||||
|
||||
struct C {
|
||||
/* ... */
|
||||
A a;
|
||||
~C() { // Compliant, noexcept by default
|
||||
/* ... */
|
||||
}
|
||||
};
|
||||
----
|
||||
|
||||
|
||||
== Resources
|
||||
|
||||
* https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c36-a-destructor-may-not-fail[{cpp} Core Guidelines C.36] - A destructor may not fail
|
||||
* https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c37-make-destructors-noexcept[{cpp} Core Guidelines C.37] - Make destructors noexcept
|
||||
|
||||
include::../../../shared_content/cfamily/exception_in_destructor.adoc[]
|
||||
|
||||
ifdef::env-github,rspecator-view[]
|
||||
|
||||
@ -122,13 +51,13 @@ Maybe remving the word "specified" in the quoted sentence would make it clearer?
|
||||
You may also see the full decision graph in CPP-2026
|
||||
|
||||
=== on 7 Nov 2018, 20:25:42 Ann Campbell wrote:
|
||||
\[~loic.joly] I think I've just realized that a couple cases aren't covered in the "raises an issue" clause.
|
||||
\[~loic.joly] I think I've just realized that a couple cases aren't covered in the "raises an issue" clause.
|
||||
|
||||
|
||||
I counter-propose:
|
||||
|
||||
____
|
||||
This rule raises an issue when
|
||||
This rule raises an issue when
|
||||
|
||||
* a destructor's default ``++noexcept++`` behavior is overridden
|
||||
* a destructor throws an exception
|
||||
@ -143,7 +72,7 @@ Because once you introduce an explicit "raises an issue" clause, you need to co
|
||||
It would be nice to detect that a destructor throws (directly or indirectly) an exception, but :
|
||||
|
||||
* This would be very hard to do correctly, and we are not doing it in this case
|
||||
* This belongs, I believe, to a more generic, yet to be written, rule: ``++noexcept++`` functions should not throw.
|
||||
* This belongs, I believe, to a more generic, yet to be written, rule: ``++noexcept++`` functions should not throw.
|
||||
|
||||
|
||||
=== on 8 Nov 2018, 17:48:45 Loïc Joly wrote:
|
||||
|
||||
45
shared_content/cfamily/exception_in_destructor.adoc
Normal file
45
shared_content/cfamily/exception_in_destructor.adoc
Normal file
@ -0,0 +1,45 @@
|
||||
== Why is this an issue?
|
||||
|
||||
Throwing an exception from a destructor may result in a call to `std::terminate`.
|
||||
|
||||
By default, compilers implicitly declare destructors as `noexcept`, so `std::terminate` is called when they exit with an exception.
|
||||
Destructors may still propagate an exception if they are explicitly declared as `noexcept(false)`.
|
||||
However, even a destructor declared as `noexcept(false)` calls `std::terminate` when it throws during stack unwinding.
|
||||
|
||||
The following example illustrates the severity of the underlying problem:
|
||||
|
||||
The destructor of a container needs to call the destructors for all managed objects.
|
||||
Suppose a call to an object's destructor throws an exception.
|
||||
In that case, there are only two _conceptual_ ways to proceed:
|
||||
|
||||
1. Abort the destruction.
|
||||
This results in a partially destroyed object and possibly many more objects whose destructors are never called.
|
||||
2. Ignore the exception and proceed with destroying the remaining objects.
|
||||
However, this potentially results in more partially destroyed objects if further destructors throw an exception.
|
||||
|
||||
Because both options are undesired, destructors should never `throw` exceptions.
|
||||
|
||||
=== What is the potential impact?
|
||||
|
||||
In most cases, throwing exceptions in destructors makes the program unreliable:
|
||||
|
||||
* If `std::terminate` is called, the program terminates in an implementation-defined, abrupt, and unclean manner.
|
||||
* The program's behavior is undefined if a standard library component (a container, an algorithm, ...) manages a user-defined object that throws an exception from its destructor.
|
||||
|
||||
include::{docdir}/how_to_fix_it.adoc[]
|
||||
|
||||
== Resources
|
||||
|
||||
=== Documentation
|
||||
|
||||
include::{docdir}/documentation.adoc[]
|
||||
|
||||
=== External coding guidelines
|
||||
|
||||
* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c36-a-destructor-may-not-fail[C.36 A destructor may not fail]
|
||||
* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/036324/CppCoreGuidelines.md#c37-make-destructors-noexcept[C.37 Make destructors noexcept]
|
||||
* MISRA {cpp}:2008, 15-5-1 - A class destructor shall not exit with an exception.
|
||||
|
||||
=== Related rules
|
||||
|
||||
include::{docdir}/related_rules.adoc[]
|
||||
Loading…
x
Reference in New Issue
Block a user