 |
Hi,
The amplification of MISRA C++:2023 rule 0.1.2 states that “This rule only applies when the function is called explicitly using function call syntax”.
Consequently, operators like “+=” are out of scope so in the following code the modification of the std::string object with “+=” is fine and the usage of append, without checking the returned reference, is a violation of this required rule:
Code: void example1() {
// Non-compliant use of std::string::append
std::string my_string;
my_string.append("foo"); // add 'foo'
my_string.append("bar"); // add 'bar'
}
void example2() {
// Compliant use of std::string::operator+=:
std::string my_string;
my_string += "foo"; // add 'foo'
my_string += "bar"; // add 'bar'
}
void example3() {
std::vector<int> numbers;
numbers.push_back(1); // Compliant to MISRA C++:2023 Rule 0.1.2
// as push_back ""returns"" void
// C++ reference: https://en.cppreference.com/w/cpp/container/vector/push_back.html
// emplace_back returns (since C++17) an iterator:
// https://en.cppreference.com/w/cpp/container/vector/emplace_back.html
numbers.emplace_back(2); // Non-compliant to MISRA C++:2023 Rule 0.1.2 (since C++17)
}
The function append of std::string (std::basic_string) and the operator+= append additional characters to the end of the string. Both return a reference to “*this”. Both throw std::length_error if the operation would cause size() to exceed max_size() and provide the strong exception safety guarantee. So, there is no real difference between using “+=” or “append” in terms of safety or code quality. If one wants to write compliant code, example1 needs to be refactored to use a void cast to ignore the result:
Code: void example1_compliant_version() {
std::string my_string;
static_cast<void>(my_string.append("foo")); // add 'foo'
static_cast<void>(my_string.append("bar")); // add 'bar'
}
That would implicitly force developers to always prefer operators like “+=” over functions like “append” as the alternatives make the code harder to read or lead to additional effort (deviations need to be justified).
If one decides to rework this kind of code to a “fluent API style” that could lead to extra copies:
Code: std::string BuildFailureMsg(std::string_view name, std::string_view reason) {
std::string msg{};
msg.reserve(name.size() + reason.size() + 26); // reserve sufficient memory
msg.append("Component ");
msg.append(name);
msg.append(" failed due to ");
msg.append(reason);
msg.append("\n");
return msg;
}
std::string BuildFailureMsgNaiveRefactor(std::string_view name,
std::string_view reason) {
std::string msg{};
msg.reserve(name.size() + reason.size() + 26); // reserve sufficient memory
return msg.append("Component ")
.append(name)
.append(" failed due to ")
.append(reason)
.append("\n"); // will create and return copy of msg (i.e. see extra "memcpy")
}
int main() {
std::string failure_msg_01 = BuildFailureMsg("Logger", "insufficient memory"); // no copies/moves
std::string failure_msg_02 = BuildFailureMsgNaiveRefactor("Logger", "insufficient memory"); // extra copy
}
Consequently, existing code that uses append needs to be refactored to use “+=”. A huge refactoring effort for large code bases, but without any real benefit.
Also, it might lead to the negative effect that developers that uses std::vector prefer “push_back” over “emplace_back”, as with C++17 std::vector::emplace_back returns a reference (example3). Other alternatives might lead to less efficient code.
Our questions:
- Is it really intended to promote the usage of operators over functions and (for std::vector and other std-library types like std::optional::emplace) to use push_back over emplace_back or are here just exceptions missing?
- As the rationale states that “it is possible to call a function without using the return value, which may be an error”, should this rule be changed to “advisory” and the title changed to “The value returned by a function should be used”.
- Independent of 1 and 2: should an additional exception for functions that use C++ exceptions to report errors added?
|
