Return "PyErr_Format" calls in "traits/ctraits.c"

[rahulporuri]

As mentioned in an earlier comment #1631 (comment), this PR returns PyErr_Format instead of explicitly returning NULL.

Checklist

• Tests
• Update API reference (docs/source/traits_api_reference)
• Update User manual (docs/source/traits_user_manual)
• Update type annotation hints in traits-stubs

[rahulporuri]

I wasn't sure of one change in this PR and CI seems to agree -

      gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -I/usr/local/opt/sqlite/include -I/usr/local/opt/sqlite/include -I/Users/runner/hostedtoolcache/Python/3.7.13/x64/include/python3.7m -c traits/ctraits.c -o build/temp.macosx-10.15-x86_64-cpython-37/traits/ctraits.o
      traits/ctraits.c:494:5: error: void function 'unknown_attribute_error' should not return a value [-Wreturn-type]
          return PyErr_Format(
          ^      ~~~~~~~~~~~~~
      traits/ctraits.c:2920:12: warning: incompatible pointer types returning 'PyObject *' (aka 'struct _object *') from a function with result type 'trait_object *' (aka 'struct _trait_object *') [-Wincompatible-pointer-types]
          return PyErr_Format(
                 ^~~~~~~~~~~~~
      1 warning and 1 error generated.

Not sure why the error is being raised but reverting it for now. One of the errors is because I was returning from a static void function.

[rahulporuri]

should this function become a static int instead of a static void like the similar invalid_attribute_error function?

[mdickinson]

I don't think that would buy us anything, except consistency. The static int on the other functions is a little odd, because the return value is mostly useless - it doesn't pass on any information. It's a minor convenience if you happen to be calling invalid_attribute_error from a function that expects to return an int, but I don't see a clear reason why that would always be the case - you might just as well be calling invalid_attribute_error from a function that expects to return a pointer instead.

[mdickinson]

LGTM. For commit 8c3bf24, there's no good reason that I can see for trait_new to be returning a trait_object* rather than a PyObject *. I think that function should be changed to have return type trait_object* (since that's what tp_new expects), and then the (newfunc) cast further down can be removed.

[mdickinson]

changed to have return type trait_object*

Gah. Sorry. I meant "changed to have return type PyObject *, of course.

[mdickinson]

You want to do the cast at the return trait line. trait->getattr and trait->setattr won't make sense for a general PyObject *.

[rahulporuri]

I think that function should be changed to have return type PyObject * (since that's what tp_new expects), and then the (newfunc) cast further down can be removed.

Is cb53eed what you meant above?

I looked at the docs for newfunc and it's not at all clear to me what it's meant to do.

[mdickinson]

I looked at the docs for newfunc and it's not at all clear to me what it's meant to do.

It's just a type declaration: typedef PyObject *(*newfunc)(PyTypeObject *, PyObject *, PyObject *);

There's a cast in ctraits.c because trait_new doesn't quite match the expected type for the tp_new slot. But fixing the return type of trait_new is a better fix here than using the cast.

[mdickinson]

LGTM