[PATCH] D151353: [DebugInfo] Add error-handling to DWARFAbbreviationDeclarationSet

[Alex Langford via Phabricator via llvm-commits]

bulbazord added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:53
   }
-  return BeginOffset != *OffsetPtr;
+  return llvm::ErrorSuccess();
 }
----------------
jhenderson wrote:
> I feel like it's stated somewhere, although I don't know where, but the preferred way of returning a success state is to do `return Error::success();`
Ah yes, I took some time to read the LLVM Programmer's Manual and they suggest doing the same: https://llvm.org/docs/ProgrammersManual.html#recoverable-errors


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:127
     DWARFAbbreviationDeclarationSet AbbrDecls;
-    if (!AbbrDecls.extract(*Data, &Offset))
+    if (auto Err = AbbrDecls.extract(*Data, &Offset)) {
+      // FIXME: We should propagate the error upwards.
----------------
jhenderson wrote:
> Is there a particular reason you're using `auto` rather than `Error` here and in the similar code below? It doesn't seem to improve readability to me.
In general, I use `auto` when I feel I can convey the type information through other means (the expression or name of variable), but I hold my opinion on the use of `auto` very weakly. I'll switch it over to use `Error`.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:56-57
   DWARFAbbreviationDeclarationSet AbbrevSet;
-  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
-  EXPECT_TRUE(DataWasExtracted);
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  ASSERT_FALSE(bool(Err));
   // The Abbreviation Declarations are in order and contiguous, so we want to
----------------
jhenderson wrote:
> This is the canonical way of testing errors. There's an equivalent invocation for testing Expected and also failure states too.
Thanks for pointing that out! I'm still trying to understand LLVM's best practices with respect to testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151353