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

[Chaoren Lin]

chaoren added inline comments.

================
Comment at: include/lldb/API/SBUnixSignals.h:68
@@ -67,2 +67,3 @@
     friend class SBProcess;
+    SBUnixSignals(const lldb::ProcessSP &process_sp);
 
----------------
labath wrote:
> 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.
This is protected: so it's not part of the public API. Actually, now that you mention it, it should probably be private: instead, since there's no inheritance.

================
Comment at: include/lldb/API/SBUnixSignals.h:78
@@ -79,1 +77,3 @@
+    lldb::UnixSignalsSP m_opaque_sp;
+    lldb::ProcessWP m_process_wp;
 };
----------------
labath wrote:
> Modifying the members ( = size of the object) is also a no-no.
We can probably do without the process pointer (depending on the answer to my 3rd question). But I'm not sure it makes sense to use a WP for UnixSignals.

================
Comment at: source/Host/common/Host.cpp:1078
@@ -1077,1 +1077,3 @@
+const UnixSignalsSP &
+Host::GetUnixSignals(const ArchSpec &arch)
 {
----------------
labath wrote:
> 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?
We can probably move this function to UnixSignals itself. The signals are usually platform specific rather than process specific, so I thought it was a little weird to still have them in Process/Utility.


http://reviews.llvm.org/D11094