[Lldb-commits] [PATCH] Move ADB communications to AdbClient class - to make it accessible by other components.

[Oleksiy Vyalov]

================
Comment at: source/Plugins/Platform/Android/AdbClient.cpp:71-75
@@ +70,7 @@
+    response.split (devices, "\n");
+    if (devices.empty ())
+    {
+        error.SetErrorString ("Wrong number of devices returned from ADB");
+        return error;
+    }
+
----------------
tberghammer wrote:
> I think it isn't an error in this scenario if no device was found
Removed.

================
Comment at: source/Plugins/Platform/Android/AdbClient.cpp:109-121
@@ +108,15 @@
+
+Error
+AdbClient::GetSerialNumber (std::string& sn)
+{
+    auto error = SendDeviceMessage ("get-serialno");
+    if (error.Fail ())
+        return error;
+
+    error = ReadResponseStatus ();
+    if (error.Fail ())
+        return error;
+
+    return ReadMessage (sn);
+}
+
----------------
tberghammer wrote:
> What is the goal of this function? I think it will return m_device_id on success and if m_device_id is not set then it will fail.
Good point - removed it. 

================
Comment at: source/Plugins/Platform/Android/AdbClient.h:35
@@ +34,3 @@
+
+    virtual ~AdbClient () = default;
+
----------------
tberghammer wrote:
> Do we need a virtual destructor?
Removed it at all.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:36-41
@@ +35,8 @@
+    AdbClient::DeviceIDList connect_devices;
+    auto error = adb.GetDevices (connect_devices);
+    if (error.Fail ())
+        return error;
+
+    assert (!connect_devices.empty ());
+    device_id = connect_devices.front ();
+    if (log)
----------------
tberghammer wrote:
> Please return an error if not exactly one device is found and remove the assert.
> 
> I think we should add asserts to check for bugs in lldb while having no connected device can be caused by almost anything.
> 
> If we found more then 1 device then (randomly) choosing one will cause some flakiness in our tests so I think it is better to report a failure here. I plan to add some way to specify the selected device from platform-android.
Makes sense - fixed.

http://reviews.llvm.org/D8535

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/