[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #1032: Add command parser

[GitBox <gi...@apache.org>]

tisonkun commented on code in PR #1032:
URL: https://github.com/apache/incubator-kvrocks/pull/1032#discussion_r1004098746


##########
src/common/status.h:
##########
@@ -285,3 +282,10 @@ struct StatusOr {
   template <typename>
   friend struct StatusOr;
 };
+
+#define GET_OR_RET(...)                                         \
+  ({                                                            \
+    auto&& status = (__VA_ARGS__);                              \
+    if (!status) return std::forward<decltype(status)>(status); \
+    std::forward<decltype(status)>(status);                     \
+  }).GetValue()

Review Comment:
   For value combination, we may implement or use a `result` type with `map`/`flatmap`, or possibly use technology described [here (coroutine_monad)](https://github.com/toby-allsopp/coroutine_monad).



##########
src/common/status.h:
##########
@@ -285,3 +282,10 @@ struct StatusOr {
   template <typename>
   friend struct StatusOr;
 };
+
+#define GET_OR_RET(...)                                         \
+  ({                                                            \
+    auto&& status = (__VA_ARGS__);                              \
+    if (!status) return std::forward<decltype(status)>(status); \
+    std::forward<decltype(status)>(status);                     \
+  }).GetValue()

Review Comment:
   According to https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html:
   
   > In G++, the result value of a statement expression undergoes array and function pointer decay, and is returned by value to the enclosing expression. For instance, if A is a class, then constructs a temporary A object to hold the result of the statement expression, and that is used to invoke Foo. Therefore the this pointer observed by Foo is not the address of a.
   
   Besides, comparing:
   
   ```c++
   const auto &cref = function(xxx);
   const auto &cref = GET_OR_RET(xxx);
   ```
   
   The latter is possible to cause `cref` a dangling reference.
   
   If we inline this trick, said for:
   
   ```c++
   ttl_ = GET_OR_RET(parser.TakeInt<int>(TTL_RANGE<int>));
   ```
   
   to
   
   ```c++
   res = parser.TakeInt<int>(TTL_RANGE<int>);
   if (!res) return res;
   ttl_ = res.GetValue();
   ```
   
   It won't increase too much code.



##########
src/common/status.h:
##########
@@ -285,3 +282,10 @@ struct StatusOr {
   template <typename>
   friend struct StatusOr;
 };
+
+#define GET_OR_RET(...)                                         \
+  ({                                                            \
+    auto&& status = (__VA_ARGS__);                              \
+    if (!status) return std::forward<decltype(status)>(status); \
+    std::forward<decltype(status)>(status);                     \
+  }).GetValue()

Review Comment:
   Besides, I cannot read:
   
   ```c++
   return GET_OR_RET(parser.template TakeInt<int>(TTL_RANGE<int>));
   ```
   
   This seems like an uncommon code snippet to me. How can I read it?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org