RFR: 7143664: Clean up OrderAccess implementations and usage

[Erik Österlund]

Hi Kim,

Thank you for your feedback.

On 22/02/15 08:43, Kim Barrett wrote:
>>> If A is-a B, then A should inherit from B.
>>>
>>> Would you agree with this?
>
> No, I don't agree.
>
> I don't see ScopedFence as being is-a ScopedFenceGeneral.  Rather, I
> see ScopedFenceGeneral as an implementation strategy for ScopedFence.

Hmm, isn't this yet another stylistic preference thing though? When I 
read just the names ScopedFence and ScopedFenceGeneral, I don't 
understand how the is-a relationship is not clear. Looking how it's used 
even more so (generalization/specialization). And the lack of such a 
relationship is the basis for your argument.

My opinion: Both their names and use make sense with inheritance and 
maybe we might want more levels in the hierarchy later, for instance for 
generalizing cpus so it does not have to be redefined in the same way 
for every operating system with the same cpu with a CPUScopedFence or 
something. Who knows... ;)

> No client code should ever deal with ScopedFenceGeneral. It's purely
> an implementation detail providing the extra indirection needed for
> the default implementation of ScopedFence, while still allowing the
> latter to be specialized by a platform.  As such, exposing it as a
> public base class is inappropriate.
>
> ScopedFenceGeneral is also poorly written as a base class.  It allows
> trivial accidental slicing in various ways.  To prevent that, none of
> it's API ought to be public.  This includes not just the explicit
> explicit prefix() and postfix(), but also the automatically generated
> ctor/dtor/copy/assign operations.  But once it has no public API it
> ceases to be interesting as a public base class.

I think there is a misconception here that one of these scoped fences 
should be used by client code and the other not. This is not the case. 
As a matter of fact, neither one of them should be used. All client code 
should use OrderAccess::* and none should use ScopedFence*.

If it bugs you that they are public which I can somehow understand, how 
about moving them both to private? Seems to immediately solve the 
permission problem in a pretty straight forward way. In the end, this is 
what I first did but then moved them out to get smaller lines haha! :)

If you agree with this you can stop reading here. If not, and you would 
prefer your variant instead, I have some comments about it:

> Below is a sketch of how I think ScopedFence should be done.  Note
> that the public ScopedFenceGeneral has been replaced with the private
> ScopedFence::Impl.

I think that by reading the names ScopedFence and ScopedFenceGeneral 
(and their inheritance relationship), it was easy to understand which 
one is the generalized variant and which one is not.

To me the name ScopedFence<T>::Impl implies this is /the one/ 
implementation of ScopedFence. But it is in fact the generalized 
implementation, which does not seem obvious. To me this is unintuitive 
and makes it harder to read.

Let's compare the two solutions.

My solution:

template<> inline void ScopedFenceGeneral<X_ACQUIRE>::postfix() { 
OrderAccess::acquire(); }
template<> inline void ScopedFence<X_ACQUIRE>::postfix() { }

Your solution:

template<> inline void ScopedFence<X_ACQUIRE>::Impl::postfix(){ 
OrderAccess::acquire(); }
template<> inline void ScopedFence<X_ACQUIRE>::postfix() { }

Would you agree it's harder to work out which one is the generalized 
behavior in your solution when just reading the code? I would prefer at 
least something like Impl -> DefaultImpl to make it more readable in 
that case.

Thank you for the comments! :)

/Erik

> ----------
> #include <iostream>
>
> enum ScopedFenceType {
>      X_ACQUIRE
>    , RELEASE_X
>    , RELEASE_X_FENCE
> };
>
> template<ScopedFenceType T>
> class ScopedFence /* : public ScopedObj */ {
>    class Impl;
>
> public:
>    ScopedFence() { prefix(); }
>    ~ScopedFence() { postfix(); }
>    void prefix() { Impl::prefix(); }
>    void postfix() { Impl::postfix(); }
> };
>
> template<ScopedFenceType T>
> struct ScopedFence<T>::Impl /* : public AllStatic */ {
>    static void prefix() { }
>    static void postfix() { }
> };
>
> // Default implementations for specific fence types
> template<> inline void ScopedFence<X_ACQUIRE>::Impl::postfix() {
>    std::cout << "ScopedFence<X_ACQUIRE>::postfix() => Impl\n";
> }
>
> template<> inline void ScopedFence<RELEASE_X>::Impl::prefix() {
>    std::cout << "ScopedFence<RELEASE_X>::prefix() => Impl\n";
> }
>
> // platform-specific override
> template<> inline void ScopedFence<X_ACQUIRE>::postfix() {
>    std::cout << "ScopedFence<X_ACQUIRE>::postfix() => override\n";
> }
>
> int main(int argc, char* argv[]) {
>    ScopedFence<X_ACQUIRE> a;
>    ScopedFence<RELEASE_X> r;
>    return 0;
> }
>
>
>
>