Page MenuHomePhabricator

[llvm-readobj] - Deprecate the unwrapOrError helper.
AbandonedPublic

Authored by grimar on Tue, Feb 11, 6:32 AM.

Details

Summary

I've noticed that we use unwrapOrError often.
It is a thing that fail on a error. It is a bit strange to me that we have something like that.

I think, that any (we have it in objdump, objcopy, but I had no chance to revew it)
dumper tool we have actually should never fail.
At least I see no reason for example why we print "<?>" for the section names in one case
and might fail with a error in another? I am not sure about GNU style where we are trying to follow GNU,
but still looks strange to me that we are able to report a error at all.

Dumper is a thing that is used to inspect a binary,
it does not make sense at all to allow it to fail.
Following the logic above I wonder why we have unwrapOrError function.
It is a corrupt for any dumper tool design IMO. (Isn't it?)

I investigated this area and at first tried to make it as deprecated:

template <class T>
LLVM_ATTRIBUTE_DEPRECATED(
    T unwrapOrError(StringRef Input, Expected<T> EO) {
      if (EO)
        return *EO;
      reportError(EO.takeError(), Input);
    },
    "XXXXX");

It kind of works, but I do not know if it is fine. I just do not
know much about it.
E.g. Can our bots fail? I observed failtures from bots before in my practice,
I do not know about the rules they use.
The approach above triggers a lot of warnings for me.
I.e. if all of warnings can trigger a failture or there is a set of allowed ones?
(Or if we have a single bot with "Treat Warning As Errors", it is enough to fail which
might break the whole idea).

Another thing I'd like to mention: llvm-readobj uses this helper in many places. I.e. it
should not be a trivial thing to fix it everywhere (and add tests I mean).
I can handle some of them, but for example I'd like to care about ELF,
but have no interest in COFF.

So, the solution I'd like to suggest: what if we just rename the unwrapOrError to unwrapOrError_DEPRECATED?
Pros: People will know it is a bad thing, either they'll try to rewrite it or to at least stop copy-pasting.
Cons: I do not know any of. A bad name? We need to get rid of this thing from all our dumper tools I believe.

Diff Detail

Event Timeline

grimar created this revision.Tue, Feb 11, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Feb 11, 6:32 AM
grimar edited the summary of this revision. (Show Details)Tue, Feb 11, 6:43 AM

Why not just get rid of it, and update the call sites?

grimar planned changes to this revision.Wed, Feb 12, 12:15 AM

Why not just get rid of it, and update the call sites?

Well... because I see 153 of them :) I think we should and I do not mind to care about at least some of them, but given the previous my experience my guess is
(I did not yet check though) that many of them are uncovered by tests. Writing tests for that amount of places might take a significant time.

Let me try to investigate it more though. May be it is not that bad how it looks like to me.

Sorry :) I just am not a fan of the _DEPRECATED name!

Sorry :) I just am not a fan of the _DEPRECATED name!

I've got it ;)

grimar abandoned this revision.Sun, Feb 16, 11:47 PM