[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:335-340
+    if (llvm::Error E = user_type_or_err.takeError()) {
+      std::string Reason = llvm::toString(std::move(E));
+      if (log)
+        log->Printf("%s", Reason.c_str());
+      return false;
+    }
----------------
labath wrote:
> How about:
> ```
> if (!user_type_or_err) {
>   LLDB_LOG_ERROR(log, user_type_or_err.takeError(), "{0}");
>   return false;
> }
> ```
I think it'd be even more convenient to include the return into the macro, as Zachary suggested.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:382-387
+  if (llvm::Error E = user_type_or_err.takeError()) {
+    std::string Reason = llvm::toString(std::move(E));
     if (log)
-      log->Printf("Persistent variable's type wasn't copied successfully");
+      log->Printf("%s", Reason.c_str());
     return false;
   }
----------------
zturner wrote:
> I wonder if we need a macro like 
> 
> ```
> #define RETURN_IF_UNEXPECTED(item, ret_val, log_categories)           \
>   if (auto E = item.takeError()) {                                    \
>     std::string Reason = llvm::toString(std::move(E));                \
>     Log *log(lldb_private::GetLogIfAllCategoriesSet(log_categories)); \
>     if (log) \
>       log->Printf("%s", Reason.c_str());
>     return ret_val;
>   }
> ```
> 
> then we say:
> 
> ```
> RETURN_IF_UNEXPECTED(user_type_or_err, false, LIBLLDB_LOG_EXPRESSIONS);
> ```
> 
> I don't have too strong of an opinion, but as this pattern becomes more pervasive, it's annoying to have to write out 5-6 lines of code every time you call a function that returns an `Expected<T>`.
Thanks for the suggestion. I've introduced a macro which is pretty similar to what you've shown here.


https://reviews.llvm.org/D43912