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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:411-415
+  Error Err = Error::success();
+
+  Err = joinErrors(std::move(Err),
+                   emitDebugSectionImpl(DI, &DWARFYAML::emitDebugInfo,
+                                        "debug_info", DebugSections));
----------------
I think you could simplify this slightly:
```
Error Err = emitDebugSectionImpl(DI, &DWARFYAML::emitDebugInfo,
                                        "debug_info", DebugSections));
```


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:862
+  } else
     llvm_unreachable("unexpected emitDWARF() call");
 
----------------
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);
```


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:289
         if (0 == strncmp(&Sec.segname[0], "__DWARF", 16)) {
-          if (0 == strncmp(&Sec.sectname[0], "__debug_str", 16)) {
-            DWARFYAML::emitDebugStr(OS, Obj.DWARF);
-          } else if (0 == strncmp(&Sec.sectname[0], "__debug_abbrev", 16)) {
-            DWARFYAML::emitDebugAbbrev(OS, Obj.DWARF);
-          } else if (0 == strncmp(&Sec.sectname[0], "__debug_aranges", 16)) {
-            DWARFYAML::emitDebugAranges(OS, Obj.DWARF);
-          } else if (0 == strncmp(&Sec.sectname[0], "__debug_ranges", 16)) {
-            DWARFYAML::emitDebugRanges(OS, Obj.DWARF);
-          } else if (0 == strncmp(&Sec.sectname[0], "__debug_pubnames", 16)) {
+          Error Err = Error::success();
+
----------------
Same comment as above - all the `joinErrors` calls don't need to happen, since there is only ever one `Error` to worry about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81356