[PATCH] D77860: [Object] Change uint32_t getSymbolFlags() to Expected<uint32_t> getSymbolFlags().

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:635
+  } else
+    // TODO: Test this error.
+    return SymbolsOrErr.takeError();
----------------
Higuoxing wrote:
> jhenderson wrote:
> > This and the below TODO in this function can be removed with appropriate testing in D77864, right?
> Yes.
Okay. I just don't see the corresponding TODOs disappearing in that change ;)


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:805
+        // TODO: Report and test this error.
+        report_fatal_error(SymFlagsOrErr.takeError());
+      SymFlags = *SymFlagsOrErr;
----------------
jhenderson wrote:
> Higuoxing wrote:
> > jhenderson wrote:
> > > FWIW, I'd actually report the error properly here, and add testing only in D77864, but it's up to you.
> > Oh, I was misunderstanding the meaning of "NFC", I was thinking reporting error here is a functional change ... 🤣
> I think you understood NFC correctly, but `report_fatal_error` reports errors as much as anything else. Might as well hook up the correct error function instead.
Don't forget to rebase your other change on top of this one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77860/new/

https://reviews.llvm.org/D77860