[Lldb-commits] [PATCH] D81210: Teach GetXcodeSDK to look in the Xcode that contains LLDB

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm:364
+    Environment env = Host::GetEnvironment();
+    std::string developer_dir = env.lookup("DEVELOPER_DIR");
+    if (developer_dir.empty()) {
----------------
Above we use `getenv`, here we use `Host::GetEnvironment`. We should pick one and be consistent. 


================
Comment at: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm:367
+      // Avoid infinite recursion GetXcodeContentsDirectory calling GetXcodeSDK.
+      FileSpec path = ::GetXcodeContentsDirectory(false);
+      if (path.RemoveLastPathComponent())
----------------
It looks like this is basically dealing with one case (shlib) of the logic in GetXcodeContentsDirectory. I would prefer to make this a separate function `GetXcodeDeveloperDirectory` or something that does just that. 

Before I refactored these functions there were several methods where we tried to share code like this. While I'm generally a strong opponent of sharing code, this lead to the pattern we see here where we need to add complexity to the "generic" function to make things work. This in turn makes things more complex and harder to understand, up till the point that we inevitable implemented something "similar but different" elsewhere. 


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

https://reviews.llvm.org/D81210