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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
