[Lldb-commits] [PATCH] D116005: [lldb] [Process/FreeBSDKernel] Introduce libkvm support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Just a couple of quick comments.

In D116005#3203077 <https://reviews.llvm.org/D116005#3203077>, @mgorny wrote:

> In D116005#3202721 <https://reviews.llvm.org/D116005#3202721>, @labath wrote:
>
>> How about factoring the two implementations into subclasses. If the classes are small, maybe you can just declare them in the cpp file...
>
> Yes, that's a nice idea. As an extra, we can easily use the real pointer types and remove all these casts.

Yep. :)



================
Comment at: lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp:29-63
+#if LLDB_ENABLE_FBSDVMCORE
+class ProcessFreeBSDKernelFVC : public ProcessFreeBSDKernel {
+public:
+  ProcessFreeBSDKernelFVC(lldb::TargetSP target_sp, lldb::ListenerSP listener,
+                          fvc_t *fvc);
 
+  ~ProcessFreeBSDKernelFVC();
----------------
put into anonymous namespace, for maximum compliance.


================
Comment at: lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp:58
+
+  const char *GetError();
+
----------------
is looks like this ought to be private


================
Comment at: lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp:86
+#if defined(__FreeBSD__)
+    kvm_t *kvm =
+        kvm_open2(executable->GetFileSpec().GetPath().c_str(),
----------------
Do you really want to do this if the fvc call was successful? What is the expected priority of the two implementations? Maybe just use return statements instead of assignments?


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

https://reviews.llvm.org/D116005