[Lldb-commits] [PATCH] D76261: [lldb/PlatformMacOSX] Be more robust in computing the SDK path with xcrun

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp:225
+  lldb_private::Status error = Host::RunShellCommand(
+      "xcrun -sdk macosx --show-sdk-path", FileSpec(), &status, &signo,
+      &output_str, std::chrono::seconds(3));
----------------
Ah. We always ask for the macos SDK?


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp:226
+      "xcrun -sdk macosx --show-sdk-path", FileSpec(), &status, &signo,
+      &output_str, std::chrono::seconds(3));
+
----------------
There's a global variable for various timeouts that we should use instead. Grep for commits with timeout in the name by me.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp:250
+  if (ComputeContentsPath(sdk))
+    return sdk;
+
----------------
This looks a lot nicer than the original function.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.h:81
+    std::string default_sdk;
+    std::string contents_path;
+    XcodeSDK(std::string sdk = {})
----------------
Can you add a comment like:
`/// path/to/Xcode.app/Developer/...` so it's easier to visualize what these are?


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.h:90
+  /// Get the SDK path from xcrun.
+  static XcodeSDK GetXcodeSDK(bool use_xcrun = true);
+
----------------
The comment and the parameter seem to contradict each other about whether xcrun is being used? :-)

Should the function be called, GetDefaultXcodeSDK?
I'm also wondering why a function with this name doesn't take a triple.


================
Comment at: lldb/unittests/Platform/PlatformMacOSXTest.cpp:22
+
+TEST(PlatformMacOSXTest, ComputeContentsPath) {
+  PlatformMacOSX::XcodeSDK standard(
----------------
Awesome!


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

https://reviews.llvm.org/D76261