Fix inheritance of hash functions, restore compare_to(), slot get_hash/get_key

dtool/src/interrogate/functionRemap.cxx

@@ -796,22 +796,6 @@ setup_properties(const InterrogateFunction &ifunc, InterfaceMaker *interface_mak
         _flags |= F_iter;
       }
 
-    } else if (fname == "__getbuffer__") {
-      if (_has_this && _parameters.size() == 3 &&
-          TypeManager::is_integer(_return_type->get_new_type()) &&
-          TypeManager::is_pointer_to_Py_buffer(_parameters[1]._remap->get_orig_type()) &&
-          TypeManager::is_integer(_parameters[2]._remap->get_orig_type())) {
-
-        _flags |= F_getbuffer;
-      }
-
-    } else if (fname == "__releasebuffer__") {
-      if (_has_this && _parameters.size() == 2 &&
-          TypeManager::is_pointer_to_Py_buffer(_parameters[1]._remap->get_orig_type())) {
-
-        _flags |= F_releasebuffer;
-      }
-
     } else if (fname == "compare_to") {
       if (_has_this && _parameters.size() == 2 &&
           TypeManager::is_integer(_return_type->get_new_type())) {
@@ -833,6 +817,12 @@ setup_properties(const InterrogateFunction &ifunc, InterfaceMaker *interface_mak
         _flags |= F_divide_float;
       }
 
+    } else if (fname == "get_key" || fname == "get_hash") {
+      if (_has_this && _parameters.size() == 1 &&
+          TypeManager::is_integer(_return_type->get_new_type())) {
+        _flags |= F_hash;
+      }
+
     } else if (fname == "operator ()" || fname == "__call__") {
       // Call operators always take keyword arguments.
       _args_type = InterfaceMaker::AT_keyword_args;

dtool/src/interrogate/functionRemap.h

@@ -94,11 +94,10 @@ public:
     F_copy_constructor   = 0x0100,
     F_explicit_self      = 0x0200,
     F_iter               = 0x0400,
-    F_getbuffer          = 0x0800,
-    F_releasebuffer      = 0x1000,
-    F_compare_to         = 0x2000,
-    F_coerce_constructor = 0x4000,
-    F_divide_float       = 0x8000,
+    F_compare_to         = 0x0800,
+    F_coerce_constructor = 0x1000,
+    F_divide_float       = 0x2000,
+    F_hash               = 0x4000,
   };
 
   typedef vector<Parameter> Parameters;

dtool/src/interrogate/interfaceMakerPythonNative.cxx

@@ -321,6 +321,7 @@ get_slotted_function_def(Object *obj, Function *func, FunctionRemap *remap,
   def._answer_location = string();
   def._wrapper_type = WT_none;
   def._min_version = 0;
+  def._keep_method = false;
 
   string method_name = func->_ifunc.get_name();
   bool is_unary_op = func->_ifunc.is_unary_op();
@@ -591,6 +592,14 @@ get_slotted_function_def(Object *obj, Function *func, FunctionRemap *remap,
   if (method_name == "__cmp__" || (remap->_flags & FunctionRemap::F_compare_to) != 0) {
     def._answer_location = "tp_compare";
     def._wrapper_type = WT_compare;
+    def._keep_method = (method_name != "__cmp__");
+    return true;
+  }
+
+  if (method_name == "__hash__" || (remap->_flags & FunctionRemap::F_hash) != 0) {
+    def._answer_location = "tp_hash";
+    def._wrapper_type = WT_hash;
+    def._keep_method = (method_name != "__hash__");
     return true;
   }
 
@@ -1397,7 +1406,6 @@ write_module(ostream &out, ostream *out_h, InterrogateModuleDef *def) {
 /////////////////////////////////////////////////////////////////////////////////////////////
 void InterfaceMakerPythonNative::
 write_module_class(ostream &out, Object *obj) {
-  bool has_local_hash = false;
   bool has_local_repr = false;
   bool has_local_str = false;
   bool has_local_richcompare = false;
@@ -1496,6 +1504,9 @@ write_module_class(ostream &out, Object *obj) {
           slots[key] = slotted_def;
           slots[key]._remaps.insert(remap);
         }
+        if (slotted_def._keep_method) {
+          has_nonslotted = true;
+        }
 
         // Python 3 doesn't support nb_divide.  It has nb_true_divide and also
         // nb_floor_divide, but they have different semantics than in C++.  Ugh.
@@ -2272,6 +2283,26 @@ write_module_class(ostream &out, Object *obj) {
         }
         break;
 
+      case WT_hash:
+        // Py_hash_t func(PyObject *self)
+        {
+          out << "//////////////////\n";
+          out << "// A wrapper function to satisfy Python's internal calling conventions.\n";
+          out << "// " << ClassName << " slot " << rfi->second._answer_location << " -> " << fname << "\n";
+          out << "//////////////////\n";
+          out << "static Py_hash_t " << def._wrapper_name << "(PyObject *self) {\n";
+          out << "  " << cClassName  << " *local_this = NULL;\n";
+          out << "  if (!Dtool_Call_ExtractThisPointer(self, Dtool_" << ClassName << ", (void **)&local_this)) {\n";
+          out << "    return -1;\n";
+          out << "  }\n\n";
+
+          FunctionRemap *remap = *def._remaps.begin();
+          vector_string params;
+          out << "  return (Py_hash_t) " << remap->call_function(out, 4, false, "local_this", params) << ";\n";
+          out << "}\n\n";
+        }
+        break;
+
       case WT_none:
         // Nothing special about the wrapper function: just write it normally.
         string fname = "static PyObject *" + def._wrapper_name + "(PyObject *self, PyObject *args, PyObject *kwds)\n";
@@ -2288,37 +2319,6 @@ write_module_class(ostream &out, Object *obj) {
       }
     }
 
-    string get_key = HasAGetKeyFunction(obj->_itype);
-    if (!get_key.empty()) {
-      out << "//////////////////\n";
-      out << "//  A LocalHash(getKey) Function for this type\n";
-      out << "//     " << ClassName << "\n";
-      out << "//////////////////\n";
-      out << "static Py_hash_t Dtool_HashKey_" << ClassName << "(PyObject *self) {\n";
-      out << "  " << cClassName << " *local_this = NULL;\n";
-      out << "  if (!Dtool_Call_ExtractThisPointer(self, Dtool_" << ClassName << ", (void **)&local_this)) {\n";
-      out << "    return -1;\n";
-      out << "  }\n\n";
-      out << "  return local_this->" << get_key << "();\n";
-      out << "}\n\n";
-      has_local_hash = true;
-    } else {
-      if (bases.size() == 0) {
-        out << "//////////////////\n";
-        out << "//  A LocalHash(This Pointer) Function for this type\n";
-        out << "//     " << ClassName << "\n";
-        out << "//////////////////\n";
-        out << "static Py_hash_t Dtool_HashKey_" << ClassName << "(PyObject *self) {\n";
-        out << "  " << cClassName << " *local_this = NULL;\n";
-        out << "  if (!Dtool_Call_ExtractThisPointer(self, Dtool_" << ClassName << ", (void **)&local_this)) {\n";
-        out << "    return -1;\n";
-        out << "  }\n\n";
-        out << "  return (Py_hash_t) local_this;\n";
-        out << "}\n\n";
-        has_local_hash = true;
-      }
-    }
-
     int need_repr = 0;
     if (slots.count("tp_repr") == 0) {
       need_repr = NeedsAReprFunction(obj->_itype);
@@ -2532,6 +2532,12 @@ write_module_class(ostream &out, Object *obj) {
     out << "};\n\n";
   }
 
+  // These fields are inherited together.  We should either write all of them
+  // or none of them so that they are inherited from DTOOL_SUPER_BASE.
+  bool has_hash_compare = (slots.count("tp_hash") != 0 ||
+                           slots.count("tp_compare") != 0 ||
+                           has_local_richcompare);
+
   bool has_parent_class = (obj->_itype.number_of_derivations() != 0);
 
   // Output the type slot tables.
@@ -2661,12 +2667,11 @@ write_module_class(ostream &out, Object *obj) {
   out << "#if PY_MAJOR_VERSION >= 3\n";
   out << "    0, // tp_reserved\n";
   out << "#else\n";
-  if (slots.count("tp_compare") != 0) {
-    write_function_slot(out, 4, slots, "tp_compare");
+  if (has_hash_compare) {
+    write_function_slot(out, 4, slots, "tp_compare",
+                        "&DTOOL_PyObject_ComparePointers");
   } else {
-    // We can/should still define a comparison in Python 2 that is more
-    // meaningful that comparing id(x) with id(y).
-    out << "    &DTOOL_PyObject_ComparePointers,\n";
+    out << "    0, // tp_compare\n";
   }
   out << "#endif\n";
 
@@ -2693,10 +2698,10 @@ write_module_class(ostream &out, Object *obj) {
   }
 
   // hashfunc tp_hash;
-  if (has_local_hash) {
-    out << "    &Dtool_HashKey_" << ClassName << ",\n";
+  if (has_hash_compare) {
+    write_function_slot(out, 4, slots, "tp_hash", "&DTOOL_PyObject_HashPointer");
   } else {
-    write_function_slot(out, 4, slots, "tp_hash");
+    out << "    0, // tp_hash\n";
   }
 
   // ternaryfunc tp_call;
@@ -2762,14 +2767,15 @@ write_module_class(ostream &out, Object *obj) {
   // richcmpfunc tp_richcompare;
   if (has_local_richcompare) {
     out << "    &Dtool_RichCompare_" << ClassName << ",\n";
-  } else if (has_local_hash) {
+  } else if (has_hash_compare) {
+    // All hashable types need to be comparable.
     out << "#if PY_MAJOR_VERSION >= 3\n";
     out << "    &DTOOL_PyObject_RichCompare,\n";
     out << "#else\n";
-    out << "    0,\n";
+    out << "    0, // tp_richcompare\n";
     out << "#endif\n";
   } else {
-    write_function_slot(out, 4, slots, "tp_richcompare");
+    out << "    0, // tp_richcompare\n";
   }
 
   // Py_ssize_t tp_weaklistoffset;
@@ -3114,7 +3120,7 @@ write_function_for_top(ostream &out, InterfaceMaker::Object *obj, InterfaceMaker
     }
 
     SlottedFunctionDef slotted_def;
-    if (!get_slotted_function_def(obj, func, remap, slotted_def)) {
+    if (!get_slotted_function_def(obj, func, remap, slotted_def) || slotted_def._keep_method) {
       // It has a non-slotted remap, so we should write it.
       has_remaps = true;
       break;
@@ -6614,47 +6620,6 @@ DoesInheritFromIsClass(const CPPStructType *inclass, const std::string &name) {
   return false;
 }
 
-////////////////////////////////////////////////////////////////////////////////////////////
-//  Function : HasAGetKeyFunction
-//
-// does the class have a supportable get_key() or get_hash() to return
-// a usable Python hash?  Returns the name of the method, or empty
-// string if there is no suitable method.
-//////////////////////////////////////////////////////////////////////////////////////////
-string InterfaceMakerPythonNative::
-HasAGetKeyFunction(const InterrogateType &itype_class) {
-  InterrogateDatabase *idb = InterrogateDatabase::get_ptr();
-
-  int num_methods = itype_class.number_of_methods();
-  int mi;
-  for (mi = 0; mi < num_methods; mi++) {
-    FunctionIndex func_index = itype_class.get_method(mi);
-    const InterrogateFunction &ifunc = idb->get_function(func_index);
-    if (ifunc.get_name() == "get_key" || ifunc.get_name() == "get_hash") {
-      if (ifunc._instances != (InterrogateFunction::Instances *)NULL) {
-        InterrogateFunction::Instances::const_iterator ii;
-        for (ii = ifunc._instances->begin();
-             ii != ifunc._instances->end();
-             ++ii) {
-          CPPInstance *cppinst = (*ii).second;
-          CPPFunctionType *cppfunc = cppinst->_type->as_function_type();
-
-          if (cppfunc != NULL) {
-            if (cppfunc->_parameters != NULL &&
-                cppfunc->_return_type != NULL &&
-                TypeManager::is_integer(cppfunc->_return_type)) {
-              if (cppfunc->_parameters->_parameters.size() == 0) {
-                return ifunc.get_name();
-              }
-            }
-          }
-        }
-      }
-    }
-  }
-  return string();
-}
-
 ////////////////////////////////////////////////////////////////////////////////////////////
 //  Function : HasAGetClassTypeFunction
 //

dtool/src/interrogate/interfaceMakerPythonNative.h

@@ -82,6 +82,7 @@ private:
     WT_inplace_ternary_operator,
     WT_traverse,
     WT_compare,
+    WT_hash,
   };
 
   // This enum is passed to the wrapper generation functions to indicate
@@ -118,6 +119,7 @@ private:
     int _min_version;
     string _wrapper_name;
     set<FunctionRemap*> _remaps;
+    bool _keep_method;
   };
 
   typedef std::map<string, SlottedFunctionDef> SlottedFunctions;
@@ -197,7 +199,6 @@ public:
   bool DoesInheritFromIsClass(const CPPStructType * inclass, const std::string &name);
   bool IsPandaTypedObject(CPPStructType * inclass) { return DoesInheritFromIsClass(inclass,"TypedObject"); };
   void write_python_instance(ostream &out, int indent_level, const std::string &return_expr, bool owns_memory, const std::string &class_name, CPPType *ctype, bool is_const);
-  string HasAGetKeyFunction(const InterrogateType &itype_class);
   bool HasAGetClassTypeFunction(const InterrogateType &itype_class);
   int NeedsAStrFunction(const InterrogateType &itype_class);
   int NeedsAReprFunction(const InterrogateType &itype_class);

dtool/src/interrogatedb/dtool_super_base.cxx

@@ -30,14 +30,6 @@ PyMethodDef Dtool_Methods_DTOOL_SUPER_BASE[] = {
   { NULL, NULL }
 };
 
-static Py_hash_t Dtool_HashKey_DTOOL_SUPER_BASE(PyObject *self) {
-  void *local_this = DTOOL_Call_GetPointerThis(self);
-  if (local_this == NULL) {
-    return -1;
-  }
-  return (Py_hash_t) local_this;
-};
-
 EXPCL_DTOOLCONFIG void Dtool_PyModuleClassInit_DTOOL_SUPER_BASE(PyObject *module) {
   static bool initdone = false;
   if (!initdone) {
@@ -87,13 +79,13 @@ EXPORT_THIS Dtool_PyTypedObject Dtool_DTOOL_SUPER_BASE = {
 #if PY_MAJOR_VERSION >= 3
     0,
 #else
-    &DTOOL_PyObject_Compare,
+    &DTOOL_PyObject_ComparePointers,
 #endif
     0,
     0,
     0,
     0,
-    &Dtool_HashKey_DTOOL_SUPER_BASE,
+    &DTOOL_PyObject_HashPointer,
     0,
     0,
     PyObject_GenericGetAttr,

dtool/src/interrogatedb/py_panda.cxx

@@ -594,40 +594,13 @@ PyObject *Dtool_AddToDictionary(PyObject *self1, PyObject *args) {
 }
 
 ///////////////////////////////////////////////////////////////////////////////////
-/*
-inline long  DTool_HashKey(PyObject * inst)
-{
-    long   outcome = (long)inst;
-    PyObject *func = PyObject_GetAttrString(inst, "__hash__");
-    if (func == NULL)
-    {
-        if (DtoolCanThisBeAPandaInstance(inst))
-            if (((Dtool_PyInstDef *)inst)->_ptr_to_object != NULL)
-                outcome =  (long)((Dtool_PyInstDef *)inst)->_ptr_to_object;
-    }
-    else
-    {
-        PyObject *res = PyEval_CallObject(func, (PyObject *)NULL);
-        Py_DECREF(func);
-        if (res == NULL)
-            return -1;
-        if (PyInt_Check(res))
-        {
-            outcome = PyInt_AsLong(res);
-            if (outcome == -1)
-                outcome = -2;
-        }
-        else
-        {
-            PyErr_SetString(PyExc_TypeError,
-                "__hash__() should return an int");
-            outcome = -1;
-        }
-        Py_DECREF(res);
-    }
-    return outcome;
+Py_hash_t DTOOL_PyObject_HashPointer(PyObject *self) {
+  if (self != NULL && DtoolCanThisBeAPandaInstance(self)) {
+    Dtool_PyInstDef * pyself = (Dtool_PyInstDef *) self;
+    return (Py_hash_t) pyself->_ptr_to_object;
+  }
+  return -1;
 }
-*/
 
 /* Compare v to w.  Return
    -1 if v <  w or exception (PyErr_Occurred() true in latter case).

dtool/src/interrogatedb/py_panda.h

@@ -489,40 +489,8 @@ EXPCL_DTOOLCONFIG PyObject *Dtool_BorrowThisReference(PyObject *self, PyObject *
 EXPCL_DTOOLCONFIG PyObject *Dtool_AddToDictionary(PyObject *self1, PyObject *args);
 
 ///////////////////////////////////////////////////////////////////////////////////
-/*
-EXPCL_DTOOLCONFIG long  DTool_HashKey(PyObject * inst)
-{
-    long   outcome = (long)inst;
-    PyObject * func = PyObject_GetAttrString(inst, "__hash__");
-    if (func == NULL)
-    {
-        if(DtoolCanThisBeAPandaInstance(inst))
-            if(((Dtool_PyInstDef *)inst)->_ptr_to_object != NULL)
-                outcome =  (long)((Dtool_PyInstDef *)inst)->_ptr_to_object;
-    }
-    else
-    {
-        PyObject *res = PyEval_CallObject(func, (PyObject *)NULL);
-        Py_DECREF(func);
-        if (res == NULL)
-            return -1;
-        if (PyInt_Check(res))
-        {
-            outcome = PyInt_AsLong(res);
-            if (outcome == -1)
-                outcome = -2;
-        }
-        else
-        {
-            PyErr_SetString(PyExc_TypeError,
-                "__hash__() should return an int");
-            outcome = -1;
-        }
-        Py_DECREF(res);
-    }
-    return outcome;
-}
-*/
+
+EXPCL_DTOOLCONFIG Py_hash_t DTOOL_PyObject_HashPointer(PyObject *obj);
 
 /* Compare v to w.  Return
    -1 if v <  w or exception (PyErr_Occurred() true in latter case).