[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:29-35
+  testing::internal::CaptureStderr();
+  auto SectionsOrErr = DWARFYAML::emitDebugSections(Yaml);
+  EXPECT_THAT_ERROR(SectionsOrErr.takeError(), Failed());
+  ASSERT_EQ(testing::internal::GetCapturedStderr(),
+            std::string("YAML:10:19: error: unknown key 'Descriptor'\n"
+                        "      Descriptor: 0x12\n"
+                        "                  ^~~~\n"));
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Higuoxing wrote:
> > > jhenderson wrote:
> > > > Rather than capturing stderr, you could use: `EXPECT_THAT_EXPECTED(SectionsOrErr, FailedWithMessage("error message"));`
> > > I think the simplest way to check the error message is capturing the stderr here because the error message is printed to the `errs()`. If we use `FailedWithMessage()` we will get `Invalid argument`.
> > Ah, I see the problem. It looks like `yaml::Input` has a default error handling that consumes and prints to `errs()` the real error, before returning an error code. in `error()`, which in turn gets passed up the stack from `emitDebugSections`.
> > 
> > Probably a separate change, but I think we can and should improve this, as it's incosistent with other error handling in `emitDebugSections`. It looks like it should be straightforward to pass in a custom error handler to the `yaml::Input` constructor used within `emitDebugSections` to handle the error (i.e. by storing it and reporting it up the tree). That'll allow us to cleanly unit test it here. A longer term improvement would have `yaml::Input` store a real `Error` rather a `std::error_code`, but that looks like it might be quite a bit of work, and probably best to leave.
> > 
> > What do you think?
> I'd like to have a custom error handler and rebase this patch on that. Besides, it's good for unit testing, right?
Exactly. Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82435