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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:635
+  } else
+    // TODO: Test this error.
+    return SymbolsOrErr.takeError();
----------------
This and the below TODO in this function can be removed with appropriate testing in D77864, right?


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:657
     }
+
     if (ESym->getType() == ELF::STT_FUNC && (ESym->st_value & 1) == 1)
----------------
There's still a blank line change here that doesn't belong.


================
Comment at: llvm/lib/ExecutionEngine/Orc/Mangling.cpp:98-99
+    if (!SymFlagsOrErr)
+      // TODO: Return and test this error.
+      report_fatal_error(SymFlagsOrErr.takeError());
+
----------------
Why not just return it already? (The TODO still belongs though)


================
Comment at: llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp:131
+        // TODO: Report and test this error.
+        report_fatal_error(SymFlagsOrErr.takeError());
+
----------------
This might as well be `ES.reportError(...)`, although the TODO for testing should remain.


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/JITSymbol.cpp:61
+    // TODO: Return and test this error.
+    report_fatal_error(SymbolFlagsOrErr.takeError());
+
----------------
Just do `return SymbolFlagsOrErr.takeError();` and adjust the TODO accordingly.


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:220
+        // TODO: Return and test this error.
+        report_fatal_error(FlagsOrErr.takeError());
+      if ((*FlagsOrErr & SymbolRef::SF_Common) ||
----------------
This function already returns an `Expected`, so I'd change this to `return FlagsOrErr.takeError();`


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:243-244
+    if (!FlagsOrErr)
+      // FIXME: Return and test this error.
+      report_fatal_error(FlagsOrErr.takeError());
 
----------------
Ditto


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:604-605
+    if (!FlagsOrErr)
+      // TODO: Return and test this error.
+      report_fatal_error(FlagsOrErr.takeError());
+    if (*FlagsOrErr & SymbolRef::SF_Common) {
----------------
This function already returns an `Error`, so let's avoid the unnecessary `report_fatal_error` and do `return FlagsOrErr.takeError();`


================
Comment at: llvm/lib/Object/ObjectFile.cpp:65
+    // TODO: Actually report errors helpfully.
+    consumeError(FlagsOrErr.takeError());
   return getSymbolValueImpl(Ref);
----------------
Why `consumeError` here instead of `report_fatal_error`?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:805
+        // TODO: Report and test this error.
+        report_fatal_error(SymFlagsOrErr.takeError());
+      SymFlags = *SymFlagsOrErr;
----------------
FWIW, I'd actually report the error properly here, and add testing only in D77864, but it's up to you.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1088
+  // Symbol Flags have been checked in the caller.
+  uint32_t SymFlags = cantFail(I->getFlags());
   if (ELFObjectFileBase *ELFObj = dyn_cast<ELFObjectFileBase>(&Obj)) {
----------------
I'm okay with the variable rename, but I'd do it in a separate commit if you want to rename it.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1219-1220
+      if (!SymFlagsOrErr)
+        // TODO: Report and test this error.
+        report_fatal_error(SymFlagsOrErr.takeError());
+      if (!DebugSyms && (*SymFlagsOrErr & SymbolRef::SF_FormatSpecific))
----------------
Same comment as above.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1935-1936
+  if (!FlagsOrErr)
+    // TODO: Report and test this error.
+    report_fatal_error(FlagsOrErr.takeError());
+  uint32_t Flags = *FlagsOrErr;
----------------
Same comment as llvm-nm.


================
Comment at: llvm/tools/llvm-size/llvm-size.cpp:205-206
+    if (!SymFlagsOrErr)
+      // TODO: Report and test this error.
+      report_fatal_error(SymFlagsOrErr.takeError());
+    if (*SymFlagsOrErr & SymbolRef::SF_Common)
----------------
Same comment as llvm-nm.


================
Comment at: llvm/tools/sancov/sancov.cpp:663
+      // TODO: Report and test this error.
+      report_fatal_error(FlagsOrErr.takeError());
+    uint32_t Flags = FlagsOrErr.get();
----------------
Let's use the existing error reporting function `failIfError` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77860