[PATCH] D81356: [ObjectYAML] Add support for error handling in DWARFYAML. NFC.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:862
+  } else
     llvm_unreachable("unexpected emitDWARF() call");
 
----------------
grimar wrote:
> grimar wrote:
> > Higuoxing wrote:
> > > jhenderson wrote:
> > > > Higuoxing wrote:
> > > > > grimar wrote:
> > > > > > My concern this starts to look a bit bulky. Perhaps something like the following would be better?
> > > > > > 
> > > > > > ```
> > > > > > uint64_t BeginOffset = OS.tell();
> > > > > > Error Err = Error::success();
> > > > > > 
> > > > > > if (Name == ".debug_str")
> > > > > >   Err = DWARFYAML::EmitDebugStr(OS, DWARF);
> > > > > > else if (Name == ".debug_aranges")
> > > > > >   Err = DWARFYAML::EmitDebugAranges(OS, DWARF);
> > > > > > ...
> > > > > > 
> > > > > > if (Err)
> > > > > >   return std::move(Err);
> > > > > > ```
> > > > > Thanks a lot!
> > > > Using `joinErrors` here and below doesn't make much sense, since there is only a single error to worry about. However, I believe you can't just assign to `Err` here either, as the original `Err` will be unchecked (even `Error::success()` needs to be checked before being overwritten/destroyed).
> > > > 
> > > > I think the thing to do is close to what @grimar suggested, but with one additional line:
> > > > ```
> > > > Error Err = Error::success();
> > > > cantFail(Err); // Mark as checked before assigning.
> > > > 
> > > > if (Name == ".debug_str")
> > > >   Err = DWARFYAML::EmitDebugStr(OS, DWARF);
> > > > else if (Name == ".debug_aranges")
> > > >   Err = DWARFYAML::EmitDebugAranges(OS, DWARF);
> > > > ...
> > > > 
> > > > if (Err)
> > > >   return std::move(Err);
> > > > ```
> > > Oh, thanks a lot. At first, I don't know how to make the `Error::success()` get checked. So I use `joinErrors()` everywhere.
> > > However, I believe you can't just assign to Err here either, as the original Err will be unchecked (even Error::success() needs to be checked before being overwritten/destroyed).
> > 
> > I think the following code will invoke and trigger the original `Err` to be checked:
> > 
> > ```
> >   /// Move-construct an error value. The newly constructed error is considered
> >   /// unchecked, even if the source error had been checked. The original error
> >   /// becomes a checked Success value, regardless of its original state.
> >   Error(Error &&Other) {
> >     setChecked(true);
> >     *this = std::move(Other);
> >   }
> > ```
> > I think the following code will invoke and trigger the original Err to be checked:
> 
> Am I wrong?
Ah, I see I am wrong now. Nevermind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81356