diff options
author | Ed Tanous <edtanous@google.com> | 2023-01-19 21:29:44 +0300 |
---|---|---|
committer | Ed Tanous <ed@tanous.net> | 2023-08-14 19:53:44 +0300 |
commit | 445fce0c3fa5385c5e712ab027a98670ca64e7d1 (patch) | |
tree | c62cce16278abfb685bc7223d10153e6021ac6ad /COMMON_ERRORS.md | |
parent | 0f3fbe52313ac668c8510e5078cd47e06ceb8c8b (diff) | |
download | bmcweb-445fce0c3fa5385c5e712ab027a98670ca64e7d1.tar.xz |
Add a new common error
While this is more of a question of style, we have enough problems with
our lambda usage that it is worth having some documentation we can point
people to in review.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I2a3f819693dfd6a9a46f92037574c5d28c3121e5
Diffstat (limited to 'COMMON_ERRORS.md')
-rw-r--r-- | COMMON_ERRORS.md | 39 |
1 files changed, 39 insertions, 0 deletions
diff --git a/COMMON_ERRORS.md b/COMMON_ERRORS.md index 70c09e3476..e114e64029 100644 --- a/COMMON_ERRORS.md +++ b/COMMON_ERRORS.md @@ -328,3 +328,42 @@ binaries. Note, another form of this error involves calling nlohmann::json::reset(), to clear an object that's already been filled in. This has the potential to clear correct data that was already filled in from other sources. + +## 14. Very long lambda callbacks + +```cpp +dbus::utility::getSubTree("/", interfaces, + [asyncResp](boost::system::error_code& ec, + MapperGetSubTreeResult& res){ + <many lines of code> + }) +``` + +Inline lambdas, while useful in some contexts, are difficult to read, and have +inconsistent formatting with tools like clang-format, which causes significant +problems in review, and in applying patchsets that might have minor conflicts. +In addition, because they are declared in a function scope, they are difficult +to unit test, and produce log messages that are difficult to read given their +unnamed nature. + +Prefer to either use std::bind_front, and a normal method to handle the return, +or a lambda that is less than 10 lines of code to handle an error inline. In +cases where std::bind_front cannot be used, such as in +sdbusplus::asio::connection::async_method_call, keep the lambda length less than +10 lines, and call the appropriate function for handling non-trivial transforms. + +```cpp +void afterGetSubTree(std::shared_ptr<bmcweb::AsyncResp>& asyncResp, + boost::system::error_code& ec, + MapperGetSubTreeResult& res){ + <many lines of code> +} + +dbus::utility::getSubTree("/xyz/openbmc_project/inventory", interfaces, + std::bind_front(afterGetSubTree, asyncResp)); +``` + +See also the +[Cpp Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f11-use-an-unnamed-lambda-if-you-need-a-simple-function-object-in-one-place-only) +for generalized guidelines on when lambdas are appropriate. The above +recommendation is aligned with the C++ Core Guidelines. |