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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
