[PATCH] D26681: [ELF] Move ELF.h to Expected<T>

[Davide Italiano via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Object/ELF.h:172
+    return make_error<StringError>("invalid section index",
+                                   object_error::invalid_section_index);
   return &Sections[Index];
----------------
rafael wrote:
> At some point this should all be inconvertibleErrorCode.  For now, can you always use object_error::parse_failed? With the string we don't need the various enums.
> 
> How about for now just adding a helper function:
> 
> static StringError createError(StringRef Err) {
>   return make_error<StringError>(Err, object_error::parse_failed);
> }
> 
> That way we only have to update one place in a followup patch.
I thought the same (about using `parse_failed` everywhere, but wanted to do in a follow-up). Anyway, it's a good idea so I did it now.


================
Comment at: include/llvm/Object/ELF.h:267
       Offset + Size > Buf.size())
-    return object_error::parse_failed;
+    return make_error<StringError>("invalid offset",
+                                   object_error::parse_failed);
----------------
Bigcheese wrote:
> invalid section offset
Thanks for auditing the message, Spency


================
Comment at: include/llvm/Object/ELF.h:387
 
   // Section table goes past end of file!
   if (SectionTableOffset + SectionTableSize > FileSize)
----------------
rafael wrote:
> You don't need this comment anymore.
Done.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:621
          SymTab->sh_type == ELF::SHT_DYNSYM);
-  ErrorOr<const Elf_Shdr *> StrTabSec = EF.getSection(SymTab->sh_link);
-  if (std::error_code EC = StrTabSec.getError())
-    return EC;
-  ErrorOr<StringRef> StrTabOrErr = EF.getStringTable(*StrTabSec);
-  if (std::error_code EC = StrTabOrErr.getError())
-    return EC;
+  auto StrTabSec = EF.getSection(SymTab->sh_link);
+  if (!StrTabSec)
----------------
rafael wrote:
> This is in tools, I wonder why it doesn't just fail fast. In any case, that is for another patch.
I agree. Not quite fond of `llvm-objdump`, I'll open a bug. 


================
Comment at: tools/llvm-readobj/ARMEHABIPrinter.h:354
+  if (!StrTableOrErr)
+    error(errorToErrorCode(StrTableOrErr.takeError()));
   StringRef StrTable = *StrTableOrErr;
----------------
rafael wrote:
> Don't we have an error that takes an Error so that you don't have to call errorToErrorCode?
Yes, we do. In fact, I used it 10 lines below, but I forgot here =)


https://reviews.llvm.org/D26681