[PATCH] D63074: [llvm-readobj/llvm-readelf] - Don't fail to dump the object if .dynsym has broken sh_link field.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1426
         DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
-        DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
+        DynamicStringTable = unwrapOrWarn(Obj->getStringTableForSymtab(Sec));
       }
----------------
I'm a little uncomfortable with this interface. `unwrapOrWarn` implies that you'll get a warning or an object, yet you actually end up with both, with the latter being default constructed. However, not all objects are default constructible, meaning that this function can't be called for those types. Furthermore, the default type must be something that can be handled further on in the code correctly (imagine how easy it would be for a crash if the return type was a pointer). I think that way lies the risk of bugs. I'd suggest following the more traditional Expected behaviour, something like this:

```
Type T;
if (Expected<Type> E = getAType())
  T = *E;
else {
  reportWarning("some message);
  T = Type(A, B);
}
```

In a `StringRef` case, what you're doing is fine, as long as an empty `StringRef` makes sense further down, but that's not always the case.


================
Comment at: tools/llvm-readobj/llvm-readobj.h:47
   }
+  template <class T> T unwrapOrWarn(Expected<T> EO) {
+    if (EO)
----------------
Blank line before the function, please.

`EO` doesn't make sense as the variable name. What does it stand for?


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

https://reviews.llvm.org/D63074