[PATCH] D74405: [llvm-readobj] - Deprecate the unwrapOrError helper.

[George Rimar via Phabricator via llvm-commits]

grimar created this revision.
grimar added reviewers: jhenderson, MaskRay.
Herald added subscribers: rupprecht, aheejin, sbc100.
Herald added a project: LLVM.

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.
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.


https://reviews.llvm.org/D74405

Files:
  llvm/tools/llvm-readobj/ARMEHABIPrinter.h
  llvm/tools/llvm-readobj/COFFDumper.cpp
  llvm/tools/llvm-readobj/ELFDumper.cpp
  llvm/tools/llvm-readobj/MachODumper.cpp
  llvm/tools/llvm-readobj/ObjDumper.cpp
  llvm/tools/llvm-readobj/WasmDumper.cpp
  llvm/tools/llvm-readobj/XCOFFDumper.cpp
  llvm/tools/llvm-readobj/llvm-readobj.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D74405.243845.patch
Type: text/x-patch
Size: 55203 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200211/c05d1f8d/attachment.bin>