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

Tue May 30 10:17:59 PDT 2023

bulbazord marked an inline comment as done.
bulbazord added a comment.

In D151353#4380119 <https://reviews.llvm.org/D151353#4380119>, @jhenderson wrote:

> LGMT. There are a couple of pre-merge tests that failed that have something to do with DWARF. It's unlikely to be related to this patch, but it might be worth double-checking before landing this.

Yeah, I saw that as well. I was going to see if I could reproduce the issue locally before landing. If not, I'll watch out for failures on the bot and go from there. Thank you for the review!

================
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:
> bulbazord wrote:
> > 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`.
> It's worth noting that LLVM has policies on `auto` usage documented in the coding standards document. I think it largely boils down to "use `auto` for iterators and where the type can be inferred from the right-hand-side of the expression", although there are some nuances to it beyond that.
I'll take a look, thanks for pointing that out.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151353