D4118: index: make node tree a Python object

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  In https://phab.mercurial-scm.org/D4118#64261, @yuja wrote:
  
  > Can you bump the cext version as this patch introduces new API?
  
  
  Done. Thanks for the reminder.
  
  > 
  > 
  >> +static int nt_init_py(nodetree *self, PyObject *args)
  >>  +{
  >>  +	PyObject *index;
  >>  +	unsigned capacity;
  >>  +	if (!PyArg_ParseTuple(args, "OI", &index, &capacity))
  >>  +		return -1;
  > 
  > It leaves self->nodes uninitialized on error, and nt_dealloc() would fail if
  >  self->nodes wasn't luckily 0. Strictly speaking, it's too late to initialize
  >  pointers in tp_init because __init__() may be called more than once, but our
  >  C types don't handle such cases.
  
  I think the default tp_new clears the memory of the object, so it won't be uninitialized (maybe that's what you mean by "luckily", but I first thought your meant it would require luck for it to be set to 0). The only reason I think that might be the case is that we haven't seen any failures because of uninitialized memory in `index->nt` before (which used to be `index->nodes`). So I don't think we need to do anything about this. Let me know if you think we should change something.
  
  > 
  > 
  >> +	return nt_init(self, (indexObject*)index, capacity);
  > 
  > We'll probably need INCREF/DECREF business for the index object.
  
  Yes, I had meant to do that but then forgot, so thanks for reminding me. Done.
  
  > I didn't review the other refcounting thingy carefully. Since it's painful
  >  to do refcounting right, an internal nodetree could be embedded in the
  >  indexObject, and a thin PyObject wrapper could be added. Just an idea.
  
  Do you mean the refcounting of the nodetree? Assuming that the previous calls to `free()` (which I changed to `PyMem_Free()`) were correct, shouldn't it still be correct after replacing those by `nt_dealloc()`?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4118

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel