labath added a comment. I like this.
The coff thingy is unfortunate, but it's not a hard limitation. It's just that nobody cared about making this work from memory (until now), and (re)loading the file from disk was the easiest solution. If my d372a8e8 <https://reviews.llvm.org/rGd372a8e8bce266bb4043e6a0bfd76c7e5bf457a5> sticks, then I think we should be able to load coff files from memory too. I would like if all tests loaded the files from memory, because it makes the code simpler, but maybe that does not need to be done here -- that can definitely be handled separately. As for the testing strategy, I think that some dedicated tests would be nice in any case, if for nothing else, then for testing some of the edge cases (opening a corrupted data buffer, opening a buffer when a file with the given name exists on disk, etc.). ================ Comment at: lldb/include/lldb/Core/ModuleSpec.h:40 m_uuid(uuid), m_object_name(), m_object_offset(0), m_object_size(FileSystem::Instance().GetByteSize(file_spec)), + m_source_mappings(), m_data(data) {} ---------------- I guess this GetByteSize call should also not fire if `data` is not set. In fact I'm not sure it should fire at all. I think at one point I tried to remove this and didn't get any failures on linux. If mac is the same then maybe we could delete it unconditionally. ================ Comment at: lldb/include/lldb/Core/ModuleSpec.h:55 m_object_mod_time(rhs.m_object_mod_time), m_source_mappings(rhs.m_source_mappings) {} ---------------- You missed the copy constructor. I think both of these could be `= default`, or just omitted completely. 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
