[PATCH] D82435: [DWARFYAML][debug_gnu_*] Add the missing context `IsGNUStyle`. NFC.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/ObjectYAML/DWARFYAMLTest.cpp:77
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_EXPECTED(SectionsOrErr, Succeeded());
+}
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Higuoxing wrote:
> > > jhenderson wrote:
> > > > Higuoxing wrote:
> > > > > jhenderson wrote:
> > > > > > I think in the two passing cases, it would be good to check the value of the `Descriptor` field in the `Entries`. Ideally, I'd also test the other fields in tha YAML, but that might be outside the scope of this change, and better deferred to a later change.
> > > > > > it would be good to check the value of the Descriptor field in the Entries
> > > > > 
> > > > > Thanks, I want to do it as well. Can we use DWARF parser in lib/DebugInfo here?
> > > > I'd be slightly reluctant to do so, as there's a risk of circular testing again. Are the actual YAML data structures not available at this point then?
> > > Yes, the actual YAML data structure isn't available yet. `DWARFYAML::emitDebugSections()` returns `StringMap<std::unique_ptr<MemoryBuffer>>`. What do you think of having another function that returns the parsed YAML structure for unit testing?
> > Would this function need to be in the main library, or can it be in the unit test? If in the library, I'd want it to be small and simple. It's generally not ideal to have too many hooks in the main library that are there purely for testing, I feel.
> What about having a helper function in this test? I think we can replace the  `DWARFYAML::emitDebugSections()` with `parseDWARFYAML()` in this unit testing file since this file is for testing the YAML syntax rather than debug sections generating.
> 
> ```
> static Expected<DWARFYAML::Data>
> parseDWARFYAML(StringRef Yaml, bool IsLittleEndian,
>                                bool Is64bit) {
>   DWARFYAML::Data Data;
>   Data.IsLittleEndian = IsLittleEndian;
>   Data.Is64bit = Is64bit;
> 
>   SMDiagnostic GeneratedDiag;
>   yaml::Input YIn(
>       Yaml, /*Ctxt=*/nullptr,
>       [](const SMDiagnostic &Diag, void *DiagContext) {
>         *static_cast<SMDiagnostic *>(DiagContext) = Diag;
>       },
>       &GeneratedDiag);
> 
>   YIn >> Data;
>   if (YIn.error())
>     return createStringError(YIn.error(), GeneratedDiag.getMessage());
> 
>   return Data;
> }
> ```
That would probably make sense, since `emitDebugSections` isn't actually used at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82435