[Lldb-commits] [PATCH] D152855: [lldb][Android] Add PlatformAndroidTest

[Kazuki Sakamoto via Phabricator via lldb-commits]

splhack added inline comments.


================
Comment at: lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp:209
 
-  return adb.ShellToFile(cmd, minutes(1), destination);
+  return adb->ShellToFile(cmd, minutes(1), destination);
 }
----------------
bulbazord wrote:
> `GetAdbClient()` calls `make_unique` which can fail. You'll want to add some checks when using all the `adb` objects in this file.
will do!


================
Comment at: lldb/source/Plugins/Platform/Android/PlatformAndroid.h:73
 
+  typedef std::unique_ptr<AdbClient> AdbClientSP;
+  virtual AdbClientSP GetAdbClient();
----------------
clayborg wrote:
> bulbazord wrote:
> > `SP` -> `shared_ptr`. Was this supposed to be `std::shared_ptr`? If it's supposed to be a unique_ptr, you probably want `AdbClientUP`.
> Using the "SP" suffix is for std::shared_ptr, "UP" is for unique pointers.
@bulbazord @clayborg thanks, and oops didn't realize UP. will fix.


================
Comment at: lldb/source/Plugins/Platform/Android/PlatformAndroid.h:74
+  typedef std::unique_ptr<AdbClient> AdbClientSP;
+  virtual AdbClientSP GetAdbClient();
+
----------------
bulbazord wrote:
> I feel a little strange about this one because it's possible to create 2 `AdbClient`s from the same PlatformAndroid ID, both are unique pointers and essentially do the same thing. Maybe `PlatformAndroid` can own an `AdbClient` that it vends through `GetAdbClient`?
Yes, it's a bit strange how PlatformAndroid currently uses AdbClient. But, the scope of this diff is to unblock us first to write PlatformAndroid unittest with gmock + virtual. We will revisit for the structure change later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152855