While putting together D86046, I ran into a sharp edge with respect to the nullability semantics of objects in the C-API.
Note that in the parse method, I have to reach into the struct and check if the ptr is null:
if (!moduleRef.ptr) {
throw SetPyError(PyExc_ValueError,
"Unable to parse module assembly (see diagnostics)");
}
I’m not sure if this is intended (whether the structs were meant to be opaque). Note also a patch I had to make to PyMlirModule to give it proper move semantics, where something similar is needed (see IRModules.h at head).
I’m questioning what the right thing here is because you do have some null-check functions (i.e. mlirRegionIsNull, mlirBlockIsNull) but not on everything. I thought I’d ask before adding corresponding methods for MlirModule. I could see different ways to go on this depending on how opaque you want these structs to be. If fully opaque, then we will need proper null-initializers and null-checkers for everything. If semi-opaque (i.e. ABI stable but you don’t want people manually reaching in), then I’ve seen macros for this sort of thing.
In general, I would like to avoid users of the bindings inspecting the body of the struct, even though I don’t expect it to change much (maybe except for const-ness). I have a mild preference for functions over macros, given that the user will have to write something to check for nullity, it’s better to be a safer option. Macros come with subtleties that increase complexity…
I think I only added *IsNull functions for elements of iterable non-indexed containers because it was the only way to traverse them, but I wasn’t trying to be exhaustive in the API.
I’m not sure we actually need a possibility to create a null object. Such objects are meant to indicate the error state and are not expected by any calls, so I don’t see where they can be useful.
Creating move constructors is hard without it. I can just change the module move constructor to use a locally maintained flag – though that comes with it’s own awkwardness.
From a practical standpoint, in the situations where I have an uninitialized or orphan copy floating around, I usually like it initialized to a null value because it makes any bugs more likely to result in an immediate segv instead of being a possibly legitimate pointer that can probably be dereferenced and produce bad results later.
If you think the struct is stable and we want to support such cases, then it is probably better to have either an inline function or macro for the zero-init and IsNull operations: they tend to get used in contexts where opaque function calls produce unoptimal code.