search

ARMmbed / core-util

DEPRECATED: Mbed 3 utilities library
Other
12 stars 17 forks source link

Add missing template specialization prototypes. #56

Closed bremoran closed 8 years ago

bremoran commented 8 years ago

Change specializations to overloads for atomic_cas

Fixes #55

bremoran commented 8 years ago

cc @bogdanm @rgrover @0xc0170

0xc0170 commented 8 years ago

Move templates into detail header

I noticed this is the first PR adding detail into our modules, is it intended as detail/internal as in boost/STL ? Looking what is there for atomic ops, looks more like it's implementation (function template definitions + some regular function declarations). If there's this detail, there should there be a namespace detail ?

bremoran commented 8 years ago

I considered a detail namespace, but it didn't seem to make sense in this instance, since the detail files provide overloads for APIs that are already in the mbed::util namespace. If they were moved to a mbed::util::detail namespace, they would simply be imported back into mbed::util. Because of that, I didn't think it was worth adding the namespace.

The goal of adding the detail folder is to reduce the complexity of the API definitions.

0xc0170 commented 8 years ago

Thanks for separating those two problems.

LGTM

Offtopic as this PR was rebased, but I believe we shall do improvements around how to structure our code (namespaces like detail for instance), we can discuss this outside of this PR. We might have be better off with _impl in the case it was here here provided (template definitions). atomic_ops.hpp (declarations), atomic_ops_impl.hpp (template definitions).

bremoran commented 8 years ago

I have changed from overloads to template specializations because of the interaction with atomic_incr. It seems that a template instantiation will choose another template instantiation in preference to an overload. I have checked with GCC and it appears that this version generates the correct code.

bremoran commented 8 years ago

Note: build was manually retriggered successfully.

bogdanm commented 8 years ago

Can you please check if the correct code is also generated with armcc?

bremoran commented 8 years ago

@bogdanm The code generated by both armcc and gcc appears to be correct. Functions that should invoke atomic_incr<uint32_t> contain calls to the specialized version of atomic_cas<uint32_t> which uses ldrex and strex. Due to template inlining, there is no atomic_incr in the output.

bogdanm commented 8 years ago

Awesome! +1