[Lldb-commits] [PATCH] D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

The idea of the patch seems fine to me. One thing that I thought about is creating a `PlatformSpec` struct that can contain all the needed configuration for `CreateInstance`/`GetOrCreate`. What do you think of this?



================
Comment at: lldb/source/API/SBDebugger.cpp:1501-1502
       PlatformList &platforms = m_opaque_sp->GetPlatformList();
-      if (PlatformSP platform_sp = platforms.GetOrCreate(platform_name_cstr))
+      if (PlatformSP platform_sp =
+              platforms.GetOrCreate(platform_name_cstr, nullptr))
         platforms.SetSelectedPlatform(platform_sp);
----------------
You could add a small comment indicating what parameter the `nullptr` is standing in for.


================
Comment at: lldb/source/API/SBPlatform.cpp:298
 
-  m_opaque_sp = Platform::Create(platform_name);
+  m_opaque_sp = Platform::Create(platform_name, nullptr, nullptr);
+}
----------------
Here too


================
Comment at: lldb/source/Interpreter/OptionGroupPlatform.cpp:13
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPythonClassWithDict.h"
 #include "lldb/Target/Platform.h"
----------------
Is it `OptionGroupPythonClassWithDict.h` you want to include here? It looks more like you'd want to include `ScriptedMetadata.h`


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:514
       process->GetTarget().GetDebugger().GetPlatformList().Create(
-          PlatformDarwinKernel::GetPluginNameStatic());
+          PlatformDarwinKernel::GetPluginNameStatic(), nullptr);
   if (platform_sp.get())
----------------
Add a small comment here for `nullptr` as well.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:973
       process->GetTarget().GetDebugger().GetPlatformList().Create(
-          PlatformDarwinKernel::GetPluginNameStatic());
+          PlatformDarwinKernel::GetPluginNameStatic(), nullptr);
   if (platform_sp)
----------------
Here too.


================
Comment at: lldb/source/Target/Platform.cpp:166-199
-// PlatformSP
-// Platform::FindPlugin (Process *process, ConstString plugin_name)
-//{
-//    PlatformCreateInstance create_callback = nullptr;
-//    if (plugin_name)
-//    {
-//        create_callback  =
----------------
Yipee! :)


================
Comment at: lldb/source/Target/Platform.cpp:2042-2043
   for (const ArchSpec &arch : archs) {
-    if (PlatformSP platform = GetOrCreate(arch, process_host_arch, nullptr))
+    if (PlatformSP platform =
+            GetOrCreate(arch, process_host_arch, nullptr, nullptr))
       candidates.push_back(platform);
----------------
Here too


================
Comment at: lldb/source/Target/Platform.cpp:2081
     ArchSpec arch;
-    PlatformSP platform_sp = create_callback(true, &arch);
+    PlatformSP platform_sp = create_callback(true, &arch, &m_debugger, nullptr);
     if (platform_sp) {
----------------
Here too.


================
Comment at: lldb/source/Target/Process.cpp:2933
       platform_sp = GetTarget().GetDebugger().GetPlatformList().GetOrCreate(
-          target_arch, process_host_arch, &platform_arch);
+          target_arch, process_host_arch, &platform_arch, nullptr);
       if (platform_sp) {
----------------
Here too.


================
Comment at: lldb/source/Target/Target.cpp:1505-1506
         if (PlatformSP arch_platform_sp =
-                GetDebugger().GetPlatformList().GetOrCreate(other, {},
-                                                            &platform_arch)) {
+                GetDebugger().GetPlatformList().GetOrCreate(
+                    other, {}, &platform_arch, nullptr)) {
           SetPlatform(arch_platform_sp);
----------------
Here


================
Comment at: lldb/source/Target/TargetList.cpp:188
         if (PlatformSP platform_for_archs_sp =
-                platform_list.GetOrCreate(archs, {}, candidates)) {
+                platform_list.GetOrCreate(archs, {}, candidates, nullptr)) {
           platform_sp = platform_for_archs_sp;
----------------
Here


================
Comment at: lldb/source/Target/TargetList.cpp:221-222
             arch, {}, ArchSpec::CompatibleMatch, nullptr)) {
-      platform_sp = platform_list.GetOrCreate(arch, {}, &platform_arch);
+      platform_sp =
+          platform_list.GetOrCreate(arch, {}, &platform_arch, nullptr);
       if (platform_sp)
----------------
Here


================
Comment at: lldb/source/Target/TargetList.cpp:233
+      platform_sp = platform_list.GetOrCreate(platform_arch, {},
+                                              &fixed_platform_arch, nullptr);
       if (platform_sp)
----------------
H


================
Comment at: lldb/source/Target/TargetList.cpp:265
+      platform_sp = debugger.GetPlatformList().GetOrCreate(specified_arch, {},
+                                                           &arch, nullptr);
   }
----------------
H


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

https://reviews.llvm.org/D139249