[llvm] [Support] Add WrappedError class (PR #191706)

[via llvm-commits]

aokblast wrote:

> New code in Support definitely should have dedicated unit testing.

Ok. I will write a unittest once we have some results from the remaining issue.
> 
> I've got a few conflicting thoughts here. My first one is that we don't really want libObject having some special error mechanism that is only leveraged by llvm-readobj, but perhaps this makes sense, because we probably want the same deduplication functionality elsewhere. Let's not worry about this one at this point.

Although you mentioned that I don’t need to worry about this for now, I think it’s still reasonable to place it in Object/Error.h, since the current consumers are all within the Object-related code.
> 
> My second thought is that maybe this needs to be more generic/flexible? At the moment, if I understand this code correctly, `WrappedError` is essentially the same as a `StringError`, but with the `Prefix` argument too. What if we want a deduplicatable `FileError`? After all, it's quite common in the tools to print a warning with the name of the object prefixing the rest of the message. As a result, we could end up with the following case: "foo.o: : ". For deduplication purposes, "foo.o" is as significant as the "key bit of message", i.e. the same message from different objects should still be printed. (This may not be applicable to llvm-readobj, due to how it's implemented, but it may be for other tools). `FileError` points to the solution here: it wraps another `ErrorInfo` instance and then delegates to that wrapped instance as needed (e.g. in `log()`). A deduplication mechanism like you're doing here would likely want to follow the same style. The deduplication could then be achieved via overloading `operator==` or similar, with the new class you're implementing ignoring the "additional context" part of the message and simply calling the wrapped `ErrorInfo`'s `operator==`. Meanwhile, `FileError` would compare its `FileName` member then call the wrapped error and so on. See the Decorator design pattern for more details.

It feels a bit odd to produce error messages like "foo.o : :", especially when "foo.o :" is part of the error context. We usually structure output as Context: Error, which reads more naturally.

If we instead use Error: Context, we would need an additional parameter to decide whether the context appears as a prefix or suffix. That feels like unnecessary complexity at this stage.

That said, representing the error as a structured Error object rather than a plain string is a good idea, since it allows us to preserve more detailed information. However, I have some concerns about how this interacts with the decorator pattern.

Take FileError as an example. It is essentially a wrapped error consisting of a filename and an inner error. The question is: should we assume that users want to deduplicate based on the filename? What if they instead want to deduplicate based on the inner error?

This introduces ambiguity in how operator== should behave, since it’s unclear which part of the error should be considered for equality.

> 
> On a related note, `WrappedError` is too generic and doesn't really indicate that the important bit is that the `Prefix` is not considered significant for deduplication. A better name might be something like `ContextualizedError` or similar.

Got it! It sounds better.

> 
> @MaskRay, what do you think about this?



https://github.com/llvm/llvm-project/pull/191706