adrian-prantl wrote: > Two high-level comments: > > * I think that the `FromError`/`clone` changes would be better of as a > separate patch(es). I have no problem with the changes themselves, but I > think it'd be better to separate the meat of this patch from the mechanical > renaming. The renames could be done either before an after the patch, as > (AFAICT) the functions are just fancy names for (copy) constructors.
I can do that. > * I think this class should accept an open class hierarchy (like llvm::Error) > does and not assume that it can enumerate all the error subclasses. I think > the only additional property that we need of the error classes is the ability > to clone themselves, which we could achieve by providing a > `ClonableErrorInfo` class with an abstract `clone` method. We can still keep > the code which converts all unclonable errors into a string error (but I'd > only do it after a copy, not immediately during construction). The thing I > want to avoid is the proliferation of very specific error types inside the > base Status class -- the mach/windows error types should arguably be defined > in the host library, and even the ExpressionError type might be better off > inside the command interpreter or something. I'm not saying you have to do > the refactor now, but I'd like to have the option to do so in the future. It really looks like the need for cloneable errors and long-term storage of errors is the key insight that came out of this patch. I'll think a bit about this as I'm splitting this patch up further. https://github.com/llvm/llvm-project/pull/106774 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
