[Lldb-commits] [PATCH] D152594: [lldb] Introduce DynamicRegisterInfo::CreateFromDict

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D152594#4410620 <https://reviews.llvm.org/D152594#4410620>, @mib wrote:

> Are we replacing all the constructors that can fail by static methods ? Wouldn't it be easier to pass an `Status&` as an extra parameter ?

It would probably be easier to do, but I don't think it actually solves the problems that I'm trying to solve.

1. Status as a construct is too easy to ignore. There are plenty of examples of methods that take `Status &` where the callers create a `Status` object only to drop it on the floor afterwards. There is no consequence for ignoring a `Status` after calling a method that can fail and I don't want this to be another instance where people can just ignore error handling if it's inconvenient. I understand that it's possible to ignore the results of  `llvm::Error` and `llvm::Expected`, but this requires deliberate intent and is a lot easier to grep for. Right now, this returns a `std::unique_ptr` but perhaps it can return some kind of `llvm::Error` or `llvm::Expected` in the future.
2. Fallible constructors are difficult to get right from an error-handling perspective. Even if we pass a `Status &` and you can enforce that all callers actually check it, the constructor still gives you back a `DynamicRegisterInfo` object. For example, take the code:

  Status error;
  m_dyn_reg_info_sp = std::make_shared<DynamicRegisterInfo>(error);
  if (error.Fail())
    return error;

In this example, even if you take the error handling path, `m_dyn_reg_info_sp` (of type `std::shared_ptr<DynamicRegisterInfo>`) is still set to point to an actual `DynamicRegisterInfo` object. Later on you might try to do something like:

  if (m_dyn_reg_info_sp) {
    // use m_dyn_reg_info_sp
  }

This block //will// execute, possibly leading to strange or difficult to debug issues. A static factory method means that if initialization fails, we do not get back an object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152594