[Lldb-commits] [PATCH] D45703: Use a utility function to speed up "process load image" for POSIX Platforms

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for the heads up. I can test the android part on Wednesday, but right now I don't see a reason why that shouldn't work there.

Overall, I like the idea of using the process class for caching some platform resources. If we end up needing to do that more often, we can think of making using some more generic container for that instead of making a pair of getters/setters for each value.

Apart from the low-level inline comments, the one high-level comment I have is that the `DoLoadImage` function is getting a bit big. I think it would help readability to split it up a bit. At least the get-cached-function-or-build-one-if-it-is-not-present part seems like it could be easily separated out into a helper function.



================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1036-1037
+    // We made a good utility function, so cache it in the process:
+    dlopen_utility_func = dlopen_utility_func_up.release();
+    process->SetLoadImageUtilityFunction(dlopen_utility_func);
+    arguments = do_dlopen_function->GetArgumentValues();
----------------
If `SetLoadImageUtilityFunction` takes a unique_ptr then you can do a `SetLoadImageUtilityFunction(std::move(dlopen_utility_func_up))` here.


================
Comment at: source/Target/Process.cpp:6251
+UtilityFunction *Process::GetLoadImageUtilityFunction(Platform *platform) {
+  if (platform != GetTarget().GetPlatform().get())
+    return nullptr;
----------------
Will this ever be true? I would not expect one to be able to change the platform of a process mid-session.


================
Comment at: source/Target/Process.cpp:6256
+
+void Process::SetLoadImageUtilityFunction(UtilityFunction *utility_func) {
+  m_dlopen_utility_func_up.reset(utility_func);
----------------
I think the argument of this function should be a unique_ptr as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D45703