nop should be used wherever ufunc->nargs appears

Done. And fixed the failures by passing back retval correctly.

Why is this no longer needed?

It was impossible for it to ever be executed in the first place: the failure paths of PyUFunc_GenericFunction and Py_UFuncGeneralizedFunction both set op (here called mps) to NULL (after xdecref'ing).

But I guess it is possible that this was in fact a latent bug... @mattip, since you put the lines there, should they be moved up to the failure paths of the functions where the actual xdecref's are done? And, if so, is there a test that could be added to ensure this actually works as intended?

Adding a line

if (mps[0] != NULL) printf("\nufunc is %s\n", ufunc->name);

and running tests yeilds over 200 times this happens in tests, for comparison ufuncs (equal, greater, ...) but also for add, inner1d, and matrix_multiply

On the other hand, I do not immediately remember why we would need the PyArray_DiscardWritebackIfCopy, but I vaguely remember a failing test that I tracked down to this call. Since then perhaps the logic has changed, I will have to go back and review where I put it in. In looking over this code, I see a few places more places with perhaps excessive use of PyArray_DiscardWritebackIfCopy and NpyIter_Close, I will go back and review it again.

@mattip - it is not guaranteed that mps[i] == NULL, just that it has been properly decref'd if there is a failure. The question is if we need the DiscardWriteBackIfCopy in the function call (for cases, I guess, where there is a failure after the iterator is allocated).

This is fine the way it is. What happened? I deprecate the old PyArray_XDECREF_ERR(obj) in favor of the new PyArray_DiscardWritebackIfCopy(obj); Py_XDECREF(obj) in changeset fd4e7b and mechanically replaced all the calls without checking whether the call to PyArray_DiscardWritebackIfCopy was needed. Indeed in this case it was not.

Thank goodness for preservation of squashed commits in github.

Any reason for this change?

I moved nin == 2 inside the if statement above, so that ufuncs with other numbers of inputs are not checked. With the check just above, the assert seemed superfluous.

Should this do the full cleanup and goto fail? Could add a fail_pre_kwargs label, if we want to avoid unnecessary cleanup.

Of course, really the huge hack section of this function should be extracted from the rest of it.

Initially I had a goto cleanup_out_op and passed on a retval, but then I found that it just became confusing, and might hinder ripping this whole part out (which, as you said, should be done, but better not in this PR...). Fortunately, here we know exactly what has been done up to this point (indeed, this is partially why I moved the nin == 2 to the if-statement; I wanted to be absolutely sure I decref'd just the right number of operands, even if someone gave their ufunc a name that matches but has more than 2 inputs).

More for completeness: See #11282