[Lldb-commits] [PATCH] D83023: [lldb/ObjectFileMachO] Fetch shared cache images from our own shared cache

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like how this has turned out. Some small requests inline.



================
Comment at: lldb/include/lldb/Host/HostInfoBase.h:107
 
+  static SharedCacheImageInfo
+  GetSharedCacheImageInfo(llvm::StringRef image_name) {
----------------
Some doxygen here? "Try to find a module with the given name in the address space of the current process?"


================
Comment at: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm:493-494
+
+  SharedCacheInfo();
+  friend class HostInfoMacOSX;
+};
----------------
The class is already in an implementation file (you could empasize that by making that an anonymous namespace: <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces>). I don't think you need to go through all that trouble to avoid someone instantiating it...


================
Comment at: lldb/unittests/Host/HostInfoTest.cpp:81-117
+  ASSERT_TRUE(llvm::isa<ObjectFileMachO>(OF));
+  EXPECT_TRUE(
+      OF->GetArchitecture().IsCompatibleMatch(HostInfo::GetArchitecture()));
+  Symtab *symtab = OF->GetSymtab();
+  ASSERT_NE(symtab, nullptr);
+  void *libobjc = dlopen("/usr/lib/libobjc.A.dylib", RTLD_LAZY);
+  ASSERT_NE(libobjc, nullptr);
----------------
This is mostly about checking the ObjectFileMachO functionality, is it not? Could you make that a ObjectFileMachO unittest?

I'm mainly trying to avoid pulling in lots of libraries into the host unittest binary. The unittest binary is a good way to ensure that that the host module does not grow external dependencies (as the unittest wouldn't link), but this would pull in pretty much everything into that binary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83023/new/

https://reviews.llvm.org/D83023