labath accepted this revision. labath marked an inline comment as done. labath added inline comments. This revision is now accepted and ready to land.
================ Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:543-544 + if (m_data.ValidOffsetForDataOfSize(offset, size)) + return DataExtractor(m_data, offset, size); + ---------------- friss wrote: > This seems to work in the test, but I have to admit that I'm not 100% sure > it's correct given the comment below about wanting to write this buffer. I think this is fine. I can't find anything wanting to write to this data. Maybe that was the case once but perhaps that code was removed now. ================ Comment at: lldb/unittests/Core/ModuleSpecTest.cpp:27 + +// This test file intentionally doesn't initialize the FileSystem. +// Everything in this file should be able to run without requiring ---------------- this is cool ================ Comment at: lldb/unittests/Core/ModuleSpecTest.cpp:30 +// any interaction with the FileSystem class; by keeping it +// uninitialized, it will assert if anythign try to interact with +// it. ---------------- s/anythign/anything ================ Comment at: lldb/unittests/Core/ModuleSpecTest.cpp:57 + auto ExpectedFile = TestFile::fromYaml(R"( +--- !ELF +FileHeader: ---------------- Maybe reduce these down? I'm not sure about others, but and elf file with a single `.text` section and no symbols should still perfectly valid. ================ Comment at: lldb/unittests/TestingSupport/TestUtilities.h:39-41 + TestFile(TestFile &&RHS) : Buffer(std::move(RHS.Buffer)) { + RHS.Buffer = llvm::None; } ---------------- Since we don't need to do cleanup anymore, we can make `Buffer` a regular `std::string` and rely on the compiler-generated move and copy operations. In fact, we may not even need the `TestFile` class at all as the yaml functions could return a ModuleSpec directly. ================ Comment at: lldb/unittests/TestingSupport/TestUtilities.h:50 private: - TestFile(llvm::StringRef Name, llvm::FileRemover &&Remover) - : Name(std::string(Name)) { - Remover.releaseFile(); - } + TestFile(std::string &&Buffer) : Buffer(Buffer) {} + ---------------- you'll need to add `Buffer(std::move(Buffer))` for the `&&` to do anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83512/new/ https://reviews.llvm.org/D83512 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits