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

[Paul Hohensee]

Hi Kim,

If we go with a fine-grained name (either way is no problem for me: after
all, we have such things as globalDefinitions.hpp), then the customary name
of the class would be IsBaseOf and of the file containing it isBaseOf.hpp.

On filling out the corner cases, I'd recommend going with Erik's minimal
implementation because (1) a lot more testing will be needed on the
filled-out implementation and (2) the VM won't be using all of the
filled-out implementation, which means we ought to create and religiously
run unit tests for features the VM doesn't use, which is a PITA (don't even
know if the testing framework will allow that).  I'd put a note to the
effect that it's by no means a complete implementation, and maybe add the
filled-out implementation as a comment in case someone needs its
functionality at some future time.

But I'm not doctrinaire about it.  If, after considering the above, you
still think the filled-out implementation is needed, it's fine with me.

Paul


On Wed, Oct 22, 2014 at 11:50 PM, Erik Österlund <erik.osterlund at lnu.se>
wrote:

> 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