[Lldb-commits] [PATCH] D117076: [lldb/Plugins] Fix ScriptedThread IndexID reporting

Mon Jan 17 03:16:26 PST 2022

labath added a comment.

That is a step in the right direction, but ideally we shouldn't by introducing new functions with double return values (value in the "real" result + error through a by-ref argument, or vice versa). We have llvm::Expected for that, and (thread id issue aside) this is actually one of the biggest reasons for using the fallible constructor idiom <https://llvm.org/docs/ProgrammersManual.html#fallible-constructors> (real constructors can't return Expecteds, but static factories can).

In that light, I am very saddened by the existence of the `ErrorWithMessage` function as it encourages one to use the incorrect/old/etc. paradigm. Like, I get that it makes it handy to work with the functions which still use the old interface, but if you really want to have it, then I'd suggest to also create one which interoperates nicely with Expected-returning functions, so that you're not tempted to create new functions with by-ref error results.

Personally, I'd say this wrapper is unnecessary for the new functions, as one of the nice (opinions can vary) things about the llvm error types is that they force you to do something with those errors. Usually, when we don't have anything better to do with these errors we at least log them. So, I'd probably skip logging the errors upon creating, and log them in the caller (as it will have to handle them somehow anyway).

================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:80
+                               ScriptedThreadInterfaceSP interface_sp,
+                               Status &error, lldb::tid_t tid,
+                               StructuredData::GenericSP script_object_sp)
----------------
this is not needed now

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117076