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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h:315
+        if (*SymbolFlagsOrErr & object::SymbolRef::SF_Undefined)
           continue;
         Expected<StringRef> SymbolName = Symbol.getName();
----------------
The following might be better, because:

1) Does not use `auto` when the type is not obvious.
2) Limits the scope of the `SymbolFlagsOrErr`

```
  if (Expected<uint32_t> SymbolFlagsOrErr = Symbol.getFlags()) {
    if (*SymbolFlagsOrErr & object::SymbolRef::SF_Undefined)
      continue;
  } else {
    // FIXME: Raise an error for bad symbols.
    consumeError(SymbolFlagsOrErr.takeError());
    continue;
  }
```


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:631
+                             const Elf_Shdr *SymSec) -> Expected<bool> {
+    if (auto SymbolsOrErr = EF.symbols(SymSec))
+      return Sym == SymbolsOrErr->begin();
----------------
The type returned is not obvious, so do not use `auto` (here and in other places where applies).


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:634
+    else
+      return SymbolsOrErr.takeError();
+  };
----------------
You do not need using `else` after `return`:

```
if (auto SymbolsOrErr = EF.symbols(SymSec))
  return Sym == SymbolsOrErr->begin();
return SymbolsOrErr.takeError();
```


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:641
+  } else
+    return IsNullSymOrErr.takeError();
+
----------------
This error is not tested I think? Hence needs a FIXME too.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:647
+  } else
+    return IsNullSymOrErr.takeError();
 
----------------
I think all the code above is overcomplicated now when we have to report errors instead of consuming it.
Probably no need to have `IsNullSymbol` as it does not make the code simpler anymore:

```
  if (Expected<typename ELFT::SymRange> SymtabSyms = EF.symbols(DotSymtabSec)) {
    // Set the <????> flag for a null symbol.
    if (ESym == SymtabSyms->begin())
      Result |= SymbolRef::SF_FormatSpecific;
  } else {
    return SymtabSyms.takeError();
  }
```


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:656
+    } else
+      return NameOrErr.takeError();
+
----------------
This change doesn't seem to belong to this patch? It is unrelared to flags.


================
Comment at: llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp:208
     for (auto &Sym : COFFObj->symbols()) {
-      if (Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)
+      auto SymFlagsOrErr = Sym.getFlags();
+      if (!SymFlagsOrErr)
----------------
```
 uint32_t SymFlags = cantFail(Sym.getFlags());
```

This and below comment applies to all other places where you've added `cantFail`.


================
Comment at: llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp:210
+      if (!SymFlagsOrErr)
+        cantFail(SymFlagsOrErr.takeError());
+      if (*SymFlagsOrErr & object::BasicSymbolRef::SF_Undefined)
----------------
Why it can't fail, btw? (needs a comment)


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:220
+        // TODO: This error should be returned to the caller and tested
+        // properly.
+        report_fatal_error(FlagsOrErr.takeError());
----------------
Lets make this and other similar `FIXME` comments shorter to make it a single line.

```
FIXME: return and test this error.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77860