[Lldb-commits] [PATCH] D84809: [lldb] Fix invalid error message in TargetList::CreateTargetInternal

Tue Jul 28 17:01:27 PDT 2020

jingham added a comment.

This LGTM with a couple of quibbles but I haven't been working on the platform support recently so we should wait till people who have done so weight in.

================
Comment at: lldb/source/Target/TargetList.cpp:175-176
+          if (module_specs.GetModuleSpecAtIndex(i, module_spec)) {
+            // See if there was a selected platform and check that first
+            // since the user may have specified it.
+            if (platform_sp) {
----------------
This comment was confusing before, but it's more confusing now because platform_sp might be the user specified platform or it might be the one passed in with --platform.

================
Comment at: lldb/source/Target/TargetList.cpp:245
           }
+          error_strm.Printf("), specify an architecture to disambiguate");
+          error.SetErrorString(error_strm.GetString());
----------------
I'm not sure why you removed mention of --platform from this error because with this patch it should now be possible to disambiguate either by specifying  --platform or --arch, right?

================
Comment at: lldb/test/API/commands/target/basic/bogus.yaml:945
+              Values:          []
+      debug_line:
+        - Length:          39
----------------
You don't use the debug info, might as well leave it out.

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

https://reviews.llvm.org/D84809