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
