[PATCH] D49964: [DWARF] Refactor DWARF classes to use unified error reporting. NFC.

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk marked 9 inline comments as done.
vleschuk added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:617
     if (!Value.extractValue(AS, Offset, FormParams))
-      return make_error<StringError>("Error extracting index attribute values.",
-                                     inconvertibleErrorCode());
+      return createStringError(inconvertibleErrorCode(),
+                               "Error extracting index attribute values.");
----------------
probinson wrote:
> jhenderson wrote:
> > You missed an inconvertibleErrorCode here...
> > 
> > Actually, looking at the function itself, it can NEVER fail (always returns true), so this is technically dead code. I'd consider changing the signature of the function to return void. But that should be a separate change. If you are going to do this, I'm okay with it continuing to be inconvertibleErrorCode, but otherwise, maybe io_error?
> Re. `extractValue()` there's a place in DWARFDie.cpp that asserts the return value is true; but that's because DIEs come from a section that has actually been parsed already.  Other places that use forms to interpret data are less (ahem) assertive, and this code isn't relying on a pre-parsed section, so I think returning an error would be appropriate.
Ok, using io_error for now.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1583
   if (!TargetLookupError.empty())
-    return make_error<StringError>(TargetLookupError, inconvertibleErrorCode());
+    return createStringError(inconvertibleErrorCode(),
+                             TargetLookupError.c_str());
----------------
jhenderson wrote:
> This should be invalid_argument, looking at the lookupTarget function (which actually should just return an Error itself rather than setting a string...)
> looking at the lookupTarget function (which actually should just return an Error itself rather than setting a string...)

However return result of lookupTarget is used below. Changing its behavior is not related to this change... Setting invalid_argument.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:51
       default:
-        return make_error<StringError>(
-            "Invalid primary CFI opcode",
-            std::make_error_code(std::errc::illegal_byte_sequence));
+        return createStringError(errc::illegal_byte_sequence,
+                                 "Invalid primary CFI opcode 0x%" PRIx8,
----------------
jhenderson wrote:
> I didn't know about this errc value. Good spot! Maybe it's worth using in one or two other cases where the input is malformed? What do you think?
Yep, used it for "Incorrectly terminated.." and "Section too small". Thanks for the tip.


Repository:
  rL LLVM

https://reviews.llvm.org/D49964