Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add C++11/14 support utility file #10868

Merged
merged 1 commit into from Jul 5, 2019

Conversation

@kjbracey-arm
Copy link
Contributor

commented Jun 19, 2019

Description

As we start trying to use new facilities, we're likely to need some more helpers.

In particular, ARM C 5 has no C++11 support in its library at all, so to avoid totally breaking it we need some backup.

For the other toolchains, we can add a few C++17/C++20/TS extensions into namespace mbed to make life a little easier.

  • For ARM C 5: C++14 type_traits subset, std::move, std::forward, std::array, std::initializer_list, std::begin, std::end, std::align, std::maxalign_t, std::aligned_storage, alignof + alignas macro replacements.
  • For ARM C 5: MBED_CONSTEXPR_FN_14 and MBED_CONSTEXPR_OBJ_14 to mark things that can only be constexpr in C++14 or later.
  • For other compilers: mbed::void_t, mbed::type_identity, mbed::conjunction, mbed::disjunction, mbed::negation, mbed::experimental::nonesuch, mbed::experimental::is_detected family, mbed::as_const, mbed::remove_cvref.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@pan-, @bulislaw

Release Notes

  • Various modern C++ forward-compatibility helpers have been added in mbed_cxxsupport.h to assist with C++11 and later code, including supporting ARM C 5 as much as possible.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from f2ce0a2 to 661ece0 Jun 19, 2019
@ciarmcom ciarmcom requested review from bulislaw, pan- and ARMmbed/mbed-os-maintainers Jun 19, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from 661ece0 to f7b74a5 Jun 20, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

The majority of content in this file is a crutch to fill in the limited C++11 support in ARM C 5, so there is at least some chance of someone being able to build master with ARM C 5 once later PRs start using C++11/14.

I could leave it out and greatly simplify it, but it would mean ARM C 5 builds will soon just not work at all.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from f7b74a5 to 5c8a1a7 Jun 20, 2019
@pan-

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Last commit looks great! And maybe it would deserve its own PR.

Few things that can be added:

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Last commit looks great! And maybe it would deserve its own PR.

I've actually dropped it from this PR for now, and agree that it would. I believe there are some glitches with backporting it to C++14 - in particular stricter enum initialization rules, so it might not quite fly.

(I'm not 100% sold on std::byte in practice yet, arguably due to lack of experience, so I'm not desperate to push it).

Few things that can be added:

* `std::begin` and `std::end` that can be used in range for loop.

Good point.

* More type traits; a lot are implemented by the compiler: http://www.keil.com/support/man/docs/armcc/armcc_chr1359124947145.htm

Indeed, thanks for pointing that out!

Just spotted that myself an hour ago. (Not by reading the documentation - by doing strings armcc | grep is_convertible, finding __is_convertible_to then Googling it and hitting the Keil manual). That will help immensely.

* `std::max_align_t` and `std::align`, `std:: alignment_of` and `std::aligned_storage`

Sorry, no. ARM C 5 does not support alignof/alignas.

* `result_of` and `invoke_result`.

Will look at those.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

BTW, ARM C 5 cannot do expression SFINAE, so the Span.h is_convertible is not SFINAE usable in ARM C 5. Which is why I was groping around checking for hidden intrinsics.

@pan-

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Sorry, no. ARM C 5 does not support alignof/alignas.

That's one more reason to implement std::aligned_storage with __attribute__((aligned)) then.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from 5c8a1a7 to f026a58 Jun 20, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Okay, added most of your suggestions.

It seems there is enough to get all the alignment things apparently working, except alignas(Type) won't work, you'll have to use alignas(alignof(Type)).

I've baulked at common_type, result_of and invoke_result - getting pretty complex.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from f026a58 to 6afda5f Jun 20, 2019
Copy link
Member

left a comment

It's not really a functionality change, more of a "feature" in a sense we enable something new in the old compiler.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@pan- are you happy with the updates ?

Copy link
Member

left a comment

Very good!
It would be nice to have some tests to static_assert results of traits and make sure compilation passes on all target; today that file isn't included anywhere and won't disrupt the CI even if there's error inside. I can help with that if you need a hand.

Note: I haven't reviewed array and what's bellow yet.

platform/mbed_cxxsupport.h Show resolved Hide resolved
platform/mbed_cxxsupport.h Show resolved Hide resolved
platform/mbed_cxxsupport.h Outdated Show resolved Hide resolved
platform/mbed_cxxsupport.h Outdated Show resolved Hide resolved
platform/mbed_cxxsupport.h Outdated Show resolved Hide resolved
} // namespace impl

template <typename T>
struct is_member_pointer : impl::is_unqualified_member_pointer<remove_cv_t<T>> { };

This comment has been minimized.

Copy link
@pan-

pan- Jun 21, 2019

Member

mbed::disjunction< is_member_function_pointer<T>, is_member_object_pointer<T>> ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jun 24, 2019

Author Contributor

Logically, that's what it is, and it's shorter to write, but I shuddered at the expanded complexity. Each of those is a very simple template specialization, leading to the very complex is_function.

Seemed weird to effectively write if ((is_member && is_function) || (is_member && !is_function)) when is_function is so complex and is_member so simple.

But I have no good feel on how to optimise for compilation speed. I guess the disjunction form less work when not instantiated, as it's a single template? But if instantiated it's dozens of times more complex.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jun 24, 2019

Author Contributor

Created a new version passing a filter template to the is member detector. Not sure if that's better.

platform/mbed_cxxsupport.h Outdated Show resolved Hide resolved

/* forward */
template <typename T>
T &&forward(remove_reference_t<T> &a) noexcept

This comment has been minimized.

Copy link
@pan-

pan- Jun 21, 2019

Member

Same as above, it may be marked as constexpr.
You should also provide the second overload:

template <typename T>
 T &&forward(remove_reference_t<T> &&a) noexcept

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jun 24, 2019

Author Contributor

Hmm, lifted that from ARM C 5 manual, which only had the first overload. I wonder if there was any reason for that, or just an editorial error.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

It's not really a functionality change, more of a "feature" in a sense we enable something new in the old compiler.

"feature" isn't on the list template :)

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Very good!
It would be nice to have some tests to static_assert results of traits and make sure compilation passes on all target; today that file isn't included anywhere and won't disrupt the CI even if there's error inside. I can help with that if you need a hand.

Thanks for scanning though this.

The CI isn't currently going to catch anything from the bulk of this anyway, as ARM C 5 is not tested. (!)

Nevertheless, some tests would be appreciated. It would be worth you doing them as a second pair of eyes - it's always better if people don't write their own tests, and as this is implementing a clear external spec, we get to cross-check interpretation of that spec.

Actual static asserts should be straightforward to pass - more problematic is SFINAE traits use in enable_if or via void_t. I haven't fully figured out what idioms do and don't work in ARM C 5 - when I've got more info, I'll stick some comments in the file reflecting my findings.

I'm somewhat torn on how much effort to expend on ARM C 5. We're liable to end up doing 90% of the work in this area for a non-supported toolchain...

@pan-

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Thanks @kjbracey-arm I will add tests for the traits you added then. On one of the project I'm working on; we're working with ARM C 5 and C++11 enabled. I've already re-implemented some of the traits we needed but a more complete shim is more than welcome.

It feels like a complete move to ARMC 5 was a bit premature as ARMC 6 hasn't caught up yet on density of the code it generates. Size really matters for customers.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from 6afda5f to 5c99a24 Jun 24, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Updated incorporating comments.

I feel bad adding 1000 lines of templates to a header (no doubt more for non ARM C 5 pulling in a real <type_traits>), doing heavens-knows-what to compilation time.

So I'm going to offset by taking 4000 lines out of Callback.h in another PR in a moment.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from 5c99a24 to c9abd82 Jun 24, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

The detection idiom is driving me slightly nuts. As far as I can see, void_t works on ARM C 5, as long as we have the fallback form with struct void_helper - it doesn't work as a plain using.

Then any idiom I construct manually with void_t seems to work, eg the first example from N4436:

template< class, class = void_t<> >
struct has_type_member : false_type { };
template< class T >
struct has_type_member<T, void_t<typename T::type>> : true_type { };

Or a has_common_type using common_type_t. (And my common_type_t also uses void_t).

But trying to do the same test via the detection idiom, eg is_detected<common_type_t, X, int>, fails. Maybe a bug in template template expansion? Haven't actually gotten any working test with detector...

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from c9abd82 to b39fb4b Jun 26, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Added common_type, corrected errors in add_pointer.

As we start trying to use new facilities, we're likely to need some more
helpers.

In particular, ARM C 5 has no C++11 support in its library at all, so
to avoid totally breaking it we need some backup.

For the other toolchains, we can add a few C++17/C++20/TS extensions
into namespace mbed to make life a little easier.

* For ARM C 5: C++14 type_traits subset, std::move, std::forward,
  std::array, std::initializer_list, std::begin, std::end,
  std::align, std::maxalign_t, std::aligned_storage,
  alignof + alignas macro replacements.
* For ARM C 5: MBED_CONSTEXPR_FN_14 and MBED_CONSTEXPR_OBJ_14 to
  mark things that can only be constexpr in C++14 or later.
* For other compilers: mbed::void_t, mbed::type_identity,
  mbed::conjunction, mbed::disjunction, mbed::negation,
  mbed::experimental::nonesuch, mbed::experimental::is_detected family,
  mbed::remove_cvref, mbed::as_const.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:cxxsupport branch from b39fb4b to 80afc3a Jun 27, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Couple more minor tweaks - simplify "add_pointer"/"add_reference", then add "referenceable" checks. Flag out the detector idiom until we can figure out how to make it work on ARM C 5.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

It feels like a complete move to ARMC 5 was a bit premature as ARMC 6 hasn't caught up yet on density of the code it generates. Size really matters for customers.

Hopefully ARMC6 gets there asap 🙏

@pan- Happy with this as it is now?

@0xc0170
0xc0170 approved these changes Jul 4, 2019
Copy link
Member

left a comment

👍 makes my head to spin hard :-)))))

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

CI started

@pan-
pan- approved these changes Jul 4, 2019
Copy link
Member

left a comment

LGTM.

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 4, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit efb84f0 into ARMmbed:master Jul 5, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8589 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.