dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land.
This LGTM, with some whitespace nits inline. In D103165#2790932 <https://reviews.llvm.org/D103165#2790932>, @t.p.northover wrote: >> Also this way llvm::thread users that don't need full-sized stacks can >> continue getting the Darwin default of smaller pages. > > I'm not sure I've implemented this, since I've just made 8MB the Darwin > default here. The case I care about is LTOBackend via `ThreadPool`. Would you > prefer me to push an optional `StackSize` argument into `ThreadPool` too and > make LTOBackend specify it? > > I kind of think this has come up often enough (it's at least the second time > I've had to fix it somewhere), and 8MB is small enough for anything running > Clang that it's better to make sure it doesn't happen again. 8MB by default is probably fine; I don't feel strongly either way. ================ Comment at: llvm/include/llvm/Support/thread.h:107 + + bool joinable() const noexcept { + return Thread != native_handle_type(); ---------------- Seems like you might as well run `git-clang-format` to pick up these changes... ================ Comment at: llvm/include/llvm/Support/thread.h:261 +#endif // LLVM_SUPPORT_THREAD_H \ No newline at end of file ---------------- Looks like it's missing a newline here at the end of the file. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103165/new/ https://reviews.llvm.org/D103165 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
