RFR: 8067790: Better support for native implementations of Atomic::cmpxchg

[Kim Barrett]

On Dec 31, 2014, at 6:12 AM, Erik Österlund <erik.osterlund at lnu.se> wrote:
> 
>> The rest of these comments only matter if, contrary to my suggestion
>> above, some additional infrastructure is thought necessary.
>> 
> 
> This is what I originally thought too and hence the current solution. But voices said that using macros for this is, I quote, "that bad".
> 
>> […]
>> 
>> This scales with the number of AtomicPlatform definitions, rather than
>> the number of specialized functions as happens with the existing code.
> 
> So your proposal is to still use ugly macros but move it to the class level instead, even though at the moment there is really only one member function that needs it. That feels a bit meh to be honest. IMO if we want to do this right, why go half the way and still rely on ugly macros?

Please lay off with the perjorative "ugly macros".  The preprocessor
is part of C++, and isn't going away.  It's a tool that can be used,
or abused, and I think this is a fine use.  To me, "ugly" is adding
800+ lines of code / tests / comments to eliminate a dozen lines of
flag macro definitions and associated #ifdef's.

If that additional infrastructure were amortized across more use-cases
then it might be more appealing.  I might even have some additional
use-cases for something along that line, though not in its present
form.  (More on that below.)

>> I think the name "IsBaseOfAndDerived" would be better as
>> "IsBaseAndDerived”.  […]
>> 
>> The current implementation of IsBaseOfAndDerived (and therefore
>> IsBaseOf) may (silently) provide incorrect answers when called with
>> incomplete types. […]
>> 
>> The actual usage in this proposed change-set even relies on this
>> flawed behavior of these metafunctions, since AtomicPlatform isn't
>> always defined. […]
>> 
> 
> So what you are saying here is that I should rename IsBaseOfAndDerived to IsBaseAndDerived to more closely resemble other external libraries we do not use. But if I do that my behaviour is "incorrect" because it is not identical to that of the external library. And therefore my change would not work if it was made "correctly”.

The proposed IsBaseOfAndDerived and IsBaseOf are being described and
placed as general utilities.  Their names are strongly reminiscent of
existing facilities in other libraries, including the (C++11) standard
library. Conforming to these external standards will avoid surprises,
especially when the proposed facilities are even described as being
the same (except for a noted corner case that is both hard to deal
with in a portable manner and not actually very interesting, so I'm
not complaining about that) in the review request email.

Additionally, I think quietly providing a potentially incorrect answer
in the case of incomplete types renders the offered code quite broken
for use as a general utility.  I think the requirement for complete
types by the well-known / standard facilities is well justified.

> On the contrary this is a well tested feature on all our supported compilers and I did not intend to engineer it to be identical to some external library if it only causes trouble rather than helping. This is our own class and it can do what we want it to do and be called what we want it to be called, and I think that is fine as long as it is well tested, which it is. At least this is my opinion. Anyone else agree?

Problem is, I think it's well tested code to solve the wrong problem.

I suggest that what we're really looking for is not a base/derived
relationship between a pair of classes, but that we actually want to
determine whether a platform-specific class exists.

As a result, I think there is more infrastructure in this change set
than is actually needed.  The present implementation of
SelectBaseClass uses IsBaseOf, but doesn't need a lot of the
functionality of that metafunction. There is no actual need for a
Base/Derived relationship between the (possibly not defined)
platform-specific class and the default platform-generic class, so the
use of IsBaseOf is an unnecessary restriction that only
serendipitously tests for defined or not, due to the previously
discussed defect in this particular implementation.  I also think the
name SelectBaseClass is perhaps overly general.

I might be able to use a similar approach in some code I've been
working on as a background task.  And while writing this reply I've
thought of another place that might benefit from something along those
lines.  Thinking about the code under review in that context, I think
some changes would make these other possible use-cases easier and
clearer. 

I believe what is actually needed is a metatfunction something like:

/**
 * Metafunction whose nested type member is Default if Specific is
 * incomplete, otherwise Specific.
 *
 * Specific and Default must both be non-cv qualified class types, and
 * must not be the same type.
 *
 * If Specific is incomplete at the point where this metafunction is
 * invoked, it must never be defined elsewhere in the program.
 */
template<typename Specific, typename Default>
struct SelectPlatformBase;

[Note: I'm not wedded to that name.]

Now, it turns out to be hard / impossible to write a generic
is_complete<T> metafunction, due to ODR.  Whether a type is complete
can vary between translation units, and even within a translation
unit. Having is_complete<T>::value have different values depending on
where it is instantiated is an ODR violation.  (One can make progress
by using anonymous namespaces and macro wrappers with __LINE__
concatenation, but there is still the fundamental question of whether
is_complete<T> really even makes semantic sense.)

However, we don't need a fully general is_complete utility.  We have a
much more restricted use-case, where we want to select an optionally
defined platform-specific class if it exists, and otherwise fall back
to a more generic class.  And we're not using this in arbitrary
contexts, but only to select a platform-specialized implementation if
such exists.

Here's a lightly tested implementation of SelectPlatformBase:

// --- SelectPlatformBase

template<bool b, typename T = void>
struct EnableIf { typedef T type; };

template<typename T>
struct EnableIf<false, T> { };

template<bool Cond, typename Then, typename Else>
struct If { typedef Then type; };

template<typename Then, typename Else>
struct If<false, Then, Else> { typedef Else type; };

// Metafunction whose nested value member is true if T is defined
// (complete), and false if T is incomplete.  T must be a non-cv
// qualified class type.  If T is incomplete at the point where this
// metafunction is invoked, it must never be defined elsewhere in the
// program.
template<typename T>
class IsClassDefined {
  typedef char yes[1];
  typedef char no[2];

  template<typename U>
  static typename EnableIf<sizeof(U) == sizeof(U), yes>::type & check(U*);
  static no& check(...);

public:
  static const bool value = sizeof(check((T*)0)) == sizeof(yes);
};

template<typename Specific, typename Default>
struct SelectPlatformBase {
  typedef typename If<
    IsClassDefined<Specific>::value, Specific, Default>::type type;
};

// --- end SelectPlatformBase

That's ~30 lines of code (+ tests and comments TBD) to do precisely
what we need, replacing ~100 lines (+ tests and comments) that have
various issues.  (If and EnableIf should probably be hoisted out as
separate utilities.)  We don't actually need SelectPlatformBase, and
could instead just directly use the If and IsClassDefined
metafunctions, but giving this functionality a name might be clearer.

However, while I think there are other use cases for this specific
utility, I still think the proposed change set as a whole is adding a
lot of code just to avoid a a few lines of macro usage.  That seems
like a poor tradeoff to me.