RFR: 8058255: Native jbyte Atomic::cmpxchg for supported x86 platforms

[Erik Österlund]

Hi Kim,

Thanks for reviewing this!

On 22 Oct 2014, at 23:00, Kim Barrett <kim.barrett at oracle.com> wrote:

> On Oct 21, 2014, at 11:55 AM, Erik Österlund <erik.osterlund at lnu.se> wrote:
>> 
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8058255
>> 
>> Webrev for the simple fix (which I intend to push initially):
>> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.macro/
>> 
>> Webrev for the inheritance/template solution (which will be pushed after and is the intended "end" result):
>> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.templates/
>> 
>> Webrev for the delta from the macro solution to the inheritance/template solution:
>> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.incremental/
> 
> I haven't had time to do a real review, but I noted a couple of things
> here.
> 
> (1) I really dislike the file name "templateIdioms.hpp".  It says
> little or nothing about what is presently in that file, and with a
> name like that it seems all to likely to become a dumping ground.  My
> own preference is for relatively fine grained header files, so that I
> can include only what I actually need in a given place.  So I'd prefer
> something like is_base_of.hpp for that class, and not sure whether the
> base_class struct also goes there or in its own file.  (Fine-grained
> would argue the former.)

My own preference is also fine grained headers but I have not seen them much in this project so I actually thought coarse grained was the wanted style and thus the intention was indeed to dump things in there. We could do like boost and call it type_traits.hpp as an umbrella include and the fine grained type_traits/is_base_of.hpp if you like that better. :)

> (2) It's been a while since I looked at it, but my recollection is
> that the Boost implementation of is_base_of is substantially more
> complex than what is presently in this file.  I don't know how much of
> that (possible? assuming my recollection is correct) additional
> complexity might be to correctly support edge cases that might not be
> properly handled by this simple implementation (I wonder about CV
> qualifiers and inaccessible or ambiguous base classes), and how much
> was to deal with deficient compilers.  Related to that, I'd like to
> seem some tests.

This is actually very close to the implementation used by boost's is_base_and_derived if you expand their macros.

About using weird classes like ambiguous classes or forward declared but not defined classes, it returns false. Ambiguous types always lead to substitution failure. CV qualifiers are not disregarded currently.

I have now written a lot of tests using STATIC_ASSERT that defines the guarantees reflected in new comments describing the contract.

> (3) The documentation and protocol for these structs is weak.  For
> example, I would make only the "value" member of is_base_of public,
> with the rest private, and comment it as something like:
> is_base_of<T1, T2>::value is true iff T1 is a base class of T2

Fixed.

> (4) C++11 is_base_of<T, T> is true for class types, and similarly for
> Boost is_base_of (which attempts to match C++11).  The is_base_of
> proposed here is false in that case.  Matching the "standard" behavior
> would be preferable.  (I think the under review is_base_of differs
> from C++11 for some combinations of CV qualifiers too, as C++11 says
> the operation is performed without regard to CV qualifiers.)


I did not intend my traits to be exact replacements of the C++11 standard is_base_of and hence have not followed their semantics and considered all of their corner cases. As a matter of fact, it was a non-goal to me.

If you want me to do that though I can give it a good go and fix is_base_of to almost completely follow the standard. With my new implementation below is_base_of<B,D>::type is true iff the types are class or union and D is either derived from B or the same type as B, disregarding CV qualifiers. The difference to the C++11 standard is that it should return false for unions but the implementation of that is /extremely/ platform dependent and I don't have access to jprt which would make implementing it extremely awkward.

Here is my new proposed implementation of is_base_of following the C++11 standard "decently" with more comments and encapsulation as requested:

/**
 * is_class<T>::value is true iff the type T is a class type.
 */
template<typename T>
class is_class_or_union {
  typedef char yes[1];
  typedef char no[2];
  
  template<typename U>
  static yes &check (int U::*);
  
  template<typename>
  static no &check (...);
  
public:
  static const bool value = (sizeof(check<T>(0)) == sizeof(yes));
};

/**
 * remove_cv<T>::type returns a type T without any CV qualifiers
 */
template<class T>
class remove_cv {
  template<typename U> struct remove_const { typedef U type; };
  template<typename U> struct remove_const<const U> { typedef U type; };

  template<typename U> struct remove_volatile { typedef U type; };
  template<typename U> struct remove_volatile<volatile U> { typedef U type; };
  
public:
  typedef typename remove_volatile<typename remove_const<T>::type>::type type;
};

/**
 * is_base_of_and_derived<Base, Derived>::value is true if Derived is a derived class of Base, disgarding their CV qualifiers.
 * Returns false for non-class types except unions.
 */
template<typename Base, typename Derived>
class is_base_of_and_derived {
    typedef char yes[1];
    typedef char no[2];
    
    template<typename T>
    static yes &check(Derived*, T);
    static no &check(Base*, int);
    
    template<typename B, typename D>
    struct is_base_of_host {
        operator B*() const;
        operator D*();
    };
    
    template<typename B, typename D>
    struct is_base_of_and_derived_internal {
        static const bool value = sizeof(check(is_base_of_host<B, D>(), int())) == sizeof(yes);
    };
    
public:
    static const bool value = is_class_or_union<Base>::value &&
        is_base_of_and_derived_internal<typename remove_cv<Base>::type, typename remove_cv<Derived>::type>::value;
};

/**
 * is_same<T,U>::value is true iff T and U are the same type.
 */
template<typename T, typename U>
class is_same {
  template<typename X, typename Y>
  struct is_same_internal {
      static const bool value = false;
  };
  
  template<typename X>
  struct is_same_internal<X, X> {
      static const bool value = true;
  };
public:
  static const bool value = is_same_internal<T, U>::value;
};

/**
 * is_base_of<Base, Derived>::value is true if either Derived is a derived class of Base 
 * disregarding CV qualifiers or Derived and Base are the same type disregarding CV qualifiers.
 * Returns false for all non-class types or unions.
 */
template <typename Base, typename Derived>
class is_base_of {
public:
  static const bool value = (is_base_of_and_derived<Base, Derived>::value ||
      is_same<typename remove_cv<Base>::type, typename remove_cv<Derived>::type>::value) && 
      is_class_or_union<Base>::value;
};

If this seems ok with you I will send a new webrev with the new is_base_of implementation, its new comments describing the contract and new tests, testing the contract in detail. I need to know though if you want me to put all of the traits in different files or not and in that case if you want them in a new subdirectory or not.

Thanks,
/Erik