labath added inline comments.
================ Comment at: source/Host/posix/HostThreadPosix.cpp:44 if (IsJoinable()) { #ifndef __ANDROID__ #ifndef __FreeBSD__ ---------------- xiaobai wrote: > labath wrote: > > xiaobai wrote: > > > aprantl wrote: > > > > What about: > > > > ``` > > > > #ifdef __ANDROID__ > > > > error.SetErrorString("HostThreadPosix::Cancel() not supported on > > > > Android"); > > > > #else > > > > #ifdef __FreeBSD__ > > > > int err = ::pthread_cancel(m_thread); > > > > error.SetError(err, eErrorTypePOSIX); > > > > #else > > > > llvm_unreachable("someone is calling HostThread::Cancel()"); > > > > #endif > > > > #endif > > > > } > > > > return error; > > > > } > > > > ``` > > > I agree with Adrian's suggestion, but I would add that you can remove one > > > of the `#endif` if you use `#elif defined(__FreeBSD__)` instead of an > > > `#else` + `#ifdef __FreeBSD__`. > > I think we can just unify the __ANDROID__ and __FreeBSD__ cases (turn both > > into unreachable). We only run lldb-server on android, and there we're > > mostly single-threaded, so there shouldn't be any thread cancelling going > > on anyway... > I don't think we can make the FreeBSD case unreachable (if I understand the > code correctly) since that's the one case when `::pthread_cancel` is actually > getting called. > > If we can make the `__ANDROID__` case unreachable, this would very easily > turn into > ``` > #if defined(__FreeBSD__) || defined(__NetBSD__) > int err = ::pthread_cancel(m_thread); > error.SetError(err, eErrorTypePOSIX); > #else > llvm_unreachable("Someone is calling HostThreadPosix::Cancel()"); > #endif > ``` > > I'm somewhat unfamiliar with how exactly this code is used on android. If I > understand correctly, you're saying it's used in lldb-server and you meant > that lldb-server is mostly single threaded so this code shouldn't get run > there? Right, sorry, I misinterpreted the ifdefs. We should then merge the android case into the !FreeBsd case like you suggest (although I'm not sure why NetBSD has appeared there now). https://reviews.llvm.org/D44056 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits