[PATCH] D86614: [DWARFYAML] Make the debug_abbrev_offset field optional.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

For some reason, I didn't get any email notification about this review. Could you let me know if there are any other patches you want me to review, please?



================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:99
+  if (Index >= DebugAbbrev.size())
+    report_fatal_error("index out of range");
+
----------------
We should probably not be using `report_fatal_error` here. Either an `Expected` needs returning up the stack, or this should be an assertion, depending on whether a user input could reach this case or not.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:450-456
+    } else {
+      if (Expected<DWARFYAML::Data::AbbrevTableInfo> AbbrevTableInfoOrErr =
+              DI.getAbbrevTableInfoByID(AbbrevTableID))
+        AbbrevTableOffset = AbbrevTableInfoOrErr->Offset;
+      else
+        consumeError(AbbrevTableInfoOrErr.takeError());
+    }
----------------
I might be misremembering the llvm-dev conversation, but I think the style is if any level of an if statement requires braces, they all do (i.e. please add them to the inner if).


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:455
+      else
+        consumeError(AbbrevTableInfoOrErr.takeError());
+    }
----------------
Please add a comment saying why it's okay to `consumeError` here rather than report it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86614