[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have one remark about the consumeError+LLDB_LOG pattern. As for whether this is better than status quo or not, I still don't have an opinion on that. :)



================
Comment at: source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:80-84
+        llvm::consumeError(std::move(err));
+        LLDB_LOG(
+            lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_EXPRESSIONS),
+            "Failed to get ClangASTContext.\nReason: {}",
+            llvm::toString(std::move(err)).c_str());
----------------
I don't believe this will work. Once you consume the error, it is left in an indeterminate state (which happens to be the "success" state, but it's best not to rely on it).
If you do want to log the error (which I do recommend), then you can use the LLDB_LOG_ERROR macro. This one will clear the error for you, and it will do so even if logging is disabled. I.e., if you log the error, there's no need for an additional `consumeError` call.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65122/new/

https://reviews.llvm.org/D65122