JosephTremoulet marked 4 inline comments as done. JosephTremoulet added inline comments.
================ Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:377-383 + // TODO: We could provide a reasonable default for ThreadContext by searching + // the Thread stream for a thread with the given ID and using its Context. + // That would require a couple changes: + // 1. We'd need to pass the whole dump around as a context argument. + // 2. We'd need to ensure that the Thread stream got processed before + // the Exception stream (or make Exception's ThreadContext required + // when the Exception stream is processed before the Thread stream). ---------------- labath wrote: > I've been thinking about this for a while now, and while that idea has some > appeal, I am not sure if this would ever really be a "good" idea. Currently, > this format allows you to easily create > syntactically-valid-but-probably-nonsensical minidumps (multiple thread list > streams, multiple threads with the same ID, etc..). All of that would be more > difficult if we started depending on strict "correctness" of other parts of > the minidump in order to compute something here. > > Even if I was doing this, I'd probably implement this differently -- make the > context always optional, but then check for consistency higher up (the > validate call of the entire minidump object or something). Anyway, maybe just > delete this todo? :) I've removed the TODO, but I reserve the right to think this would be a good idea :). Speficially because I think you could still model in YAML anything you could put in a minidump file, by explicitly providing the ThreadContext even when it has a default. (This is in contrast to the other validation stuff I had in earlier revisions, where I was just misunderstanding the point of yaml validation -- so thanks for explaining it!) ================ Comment at: llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp:310-316 + ASSERT_FALSE(ExpectedFile); + auto Unhandled = + handleErrors(ExpectedFile.takeError(), [](const StringError &error) { + EXPECT_EQ(static_cast<int>(std::errc::invalid_argument), + error.convertToErrorCode().value()); + }); + EXPECT_THAT_ERROR(std::move(Unhandled), Succeeded()); ---------------- labath wrote: > Maybe: > ``` > EXPECT_THAT_EXPECTED(ExpectedFile, Failed<StringError>( > testing::Property(&StringError::convertToErrorCode, > make_error_code(errc::invalid_argument)))); > ``` > ? > > Though, this might actually be best off as a lit test where you just > FileCheck the exact error message. Moved to lit, thanks for the suggestion. The obvious place seemed to be llvm/test/ObjectYAML, but there weren't any minidump tests there so I added a minidump subdirectory for the new test. Please let me know if there was a better place that I just overlooked. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68657/new/ https://reviews.llvm.org/D68657 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits