[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:62
+  /// Return the address based on a given index.
+  Optional<uint64_t> getAddrEntry(uint32_t Index) const {
+    if (Index < Addrs.size())
----------------
vleschuk wrote:
> jhenderson wrote:
> > I'm wondering if this should return an Error instance instead of Optional for the out-of-range case. It seems like a non-existent index request would indicate a malformed section, right?
> I think that returning None is also a good indication of an error. Also non-existent index can indicate an error in upper-level logic. And just IMHO for getters like this returning a requested data as function result looks better that returning it in its argument:
> 
> ```
> Error getAddrEntry(uint32_t Index, uint64_t &Result);
> ```
Sorry, I should have said "Expected<uint64_t>":
`Expected<uint64_t> getAddrEntry(uint32_t Index);`

(Error would be used where the return type would be void or where bool or an error code would be returned). To me, Optional indicates that a "no result" is a normal and reasonable usage of this interface, whereas Expected indicates that it is bad usage of the function (whether due to malformed input or due to bad upper-level logic). It also ensures that the result must be tested for error, and cannot be ignored.


Repository:
  rL LLVM

https://reviews.llvm.org/D49676