clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed.
A few things to fix but nothing serious. Nice patch! We also need a test for this. I would add a test to lldb/test/API/functionalities/archives/TestBSDArchives.py ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:236-237 m_object_name_to_index_map.Sort(); } + if (str == ThinArchiveMagic) { + Object obj; ---------------- ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:261 + // Extract string table + str.assign((const char *)data.GetData(&offset, obj.size), obj.size); + obj.Clear(); ---------------- No need to make a copy of this data, just make a string ref to it: ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:266 + do { + offset = obj.ExtractFromThin(data, offset, llvm::StringRef(str)); + if (offset == LLDB_INVALID_OFFSET) ---------------- ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:429 length)); + container_up->m_archive_type = archive_type; ---------------- Add "archive_type" as a constructor argument for ObjectContainerBSDArchive instead of setting this afterward. ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:453 container_up->SetArchive(archive_sp); + container_up->m_archive_type = archive_sp->GetArchiveType(); return container_up.release(); ---------------- This shouldn't be needed right? If we already had the container cached, it should have the archive type set correctly already. ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:465 const char *armag = (const char *)data.PeekData(offset, sizeof(ar_hdr)); if (armag && ::strncmp(armag, ARMAG, SARMAG) == 0) { armag += offsetof(struct ar_hdr, ar_fmag) + SARMAG; ---------------- Now that we are checking two things, we can clean up this code with an early return if "armag" is null: ================ Comment at: lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp:469-471 } - return false; + if (armag && + ::strncmp(armag, ThinArchiveMagic, strlen(ThinArchiveMagic)) == 0) { ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126464/new/ https://reviews.llvm.org/D126464 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits