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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
