[PATCH] D80722: [ObjectYAML][DWARF] Make the `PubSection` optional.

[Xing GUO via Phabricator via llvm-commits]

Higuoxing marked 2 inline comments as done.
Higuoxing added inline comments.


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:299
+                     Obj.DWARF.PubNames)
+            DWARFYAML::EmitPubSection(OS, *Obj.DWARF.PubNames,
                                       Obj.IsLittleEndian);
----------------
grimar wrote:
> I think you should do this:
> 
> ```
> ...
> } else if (0 == strncmp(&Sec.sectname[0], "__debug_pubnames", 16) {
>   if (Obj.DWARF.PubNames)
>     DWARFYAML::EmitPubSection(OS, *Obj.DWARF.PubNames,  Obj.IsLittleEndian);
> } else
> ....
> ```
> 
> Otherwise the code will continue doing `strncmp` for the following `else` branches, what is
> probably not the desired flow.
Got it! Thanks for the suggestion!


================
Comment at: llvm/tools/obj2yaml/dwarf2yaml.cpp:149
+    Y.PubTypes.emplace(/*IsGNUStyle=*/false);
+    dumpPubSection(DCtx, *Y.PubTypes, PubTypes);
+  }
----------------
grimar wrote:
> Thoughts (not for this patch):
> 
> Looking on this, I seems it would be better to change the `dumpPubSection` to
> something like:
> 
> ```
> static DWARFYAML::PubSection dumpPubSection(DWARFContext &DCtx, DWARFSection Section, bool IsGNU);
> ```
> 
> It should simplify the code here  and also will allow to switch to return `Expected<DWARFYAML::PubSection>` later.
Thanks! I will do it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80722