[llvm] [Support] Add WrappedError class (PR #191706)
James Henderson via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 13 02:23:28 PDT 2026
https://github.com/jh7370 commented:
New code in Support definitely should have dedicated unit testing.
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.
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: <some additional context>: <key bit of message>". 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.
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.
@MaskRay, what do you think about this?
https://github.com/llvm/llvm-project/pull/191706
More information about the llvm-commits
mailing list