This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Threading support: Externalize hardware_concurrency()
Needs ReviewPublic

Authored by rmaprath on Feb 9 2017, 4:20 AM.

Details

Reviewers
compnerd
EricWF
Summary

Another one of those platform-dependent methods which should live behind the threading API.

Diff Detail

Event Timeline

rmaprath created this revision.Feb 9 2017, 4:20 AM
rmaprath updated this revision to Diff 88189.Feb 13 2017, 6:05 AM
rmaprath retitled this revision from Threading support: Externalize hardware_concurrency() to [libcxx] Threading support: Externalize hardware_concurrency().

Re-based on trunk.

EricWF requested changes to this revision.Feb 16 2017, 5:36 PM

Same comment as D29818

I really don't like this change (and D29818) because they start to include a ton of extra headers, and because they lift complex configuration logic into what should be a very simple header.
These functions are much better suited to being define out-of-line for a number of reasons.

Is there another way we can allow "external implementations" of these functions without lifting everything into a header?

This revision now requires changes to proceed.Feb 16 2017, 5:37 PM
rmaprath updated this revision to Diff 89664.Feb 24 2017, 6:38 AM
rmaprath edited edge metadata.

Different take on the patch: Externalize this function only for the externally-thread-api variant.

This is much less intrusive. Note that I haven't added the declaration of __libcpp_thread_hw_concurrency() into __threading_support because it doesn't belong there (needs to be provided through a custom __external_threading header instead).

There is no easy way to test this apart from building an actual external-thread-api libc++ variant. We could do some form of testing with the external-thread-library configuration, but it still requires some not-so-pretty changes that is probably best avoided.