[Lldb-commits] [PATCH] D11094: Refactor Unix signals.

[Pavel Labath]

labath added a subscriber: labath.
labath added a comment.




================
Comment at: include/lldb/API/SBUnixSignals.h:68
@@ -67,2 +67,3 @@
     friend class SBProcess;
+    SBUnixSignals(const lldb::ProcessSP &process_sp);
 
----------------
You are changing the public API here. We are maintaining binary compatibility wrt. SBAPI. You can see <http://lldb.llvm.org/SB-api-coding-rules.html> for details, but the short version is that you can add signatures, but not modfy or remove existing ones.

================
Comment at: include/lldb/API/SBUnixSignals.h:78
@@ -79,1 +77,3 @@
+    lldb::UnixSignalsSP m_opaque_sp;
+    lldb::ProcessWP m_process_wp;
 };
----------------
Modifying the members ( = size of the object) is also a no-no.

================
Comment at: source/Commands/CommandObjectProcess.cpp:1833
@@ -1832,3 +1832,3 @@
         size_t num_args = signal_args.GetArgumentCount();
-        UnixSignals &signals = process_sp->GetUnixSignals();
+        UnixSignalsSP signals = process_sp->GetUnixSignals();
         int num_signals_set = 0;
----------------
Please rename to signals_sp, per coding conventions.

================
Comment at: source/Host/common/Host.cpp:1078
@@ -1077,1 +1077,3 @@
+const UnixSignalsSP &
+Host::GetUnixSignals(const ArchSpec &arch)
 {
----------------
I am not sure this function belongs to the Host layer anymore (which I understand as providing information about the host lldb is running on), since it now provides signals for any platform. Perhaps if we move this function to process/Utility then we wouldn't have the host layer calling back into other stuff (was this the reason for your question #2?). What do you think?


http://reviews.llvm.org/D11094