self pointer parameters should have an is_nullable field with a false value

[tiawl]

Hi,

I am opening this issue to report what I think is a bug. I am currently working on the backend_conversion branch (with the type_comprehension branch features). Currently when using this command line:

python3 dear_bindings/dear_bindings.py --output cimgui imgui/imgui.h

The generated cimgui.json, for C++ structs' methods translated as C functions, looks like this for the self parameter:

"arguments": [
{
  "name": "self",
  "type": {
    "declaration": "ImDrawList*",
    "description": {
      "kind": "Pointer",
      "inner_type": {
        "kind": "User",
        "name": "ImDrawList"
      }
    }
  },
  "is_array": false,
  "is_varargs": false
},

I really think this parameter should at least contain a is_nullable set to a false value. We do not want a self parameter with a null value for a C++ method.

I would like also submit another field for the self parameters. You maybe feel that I am insistent but adding a field to say that we also do not want this parameter points to several items would be great and "conservative".

Maybe I am wrong but just glancing your code, I think that the few modifications to add these features should be done in the src/modifiers/mod_flatten_class_functions.py. If you are busy during the next weeks (and also because I am opening a lot of issues that implied changes during the last weeks), I am OK to open a pull request.

Thank you.

[tiawl]

I also think that generating a field is_method_of: STRUCT_NAME (None if it is not originally a method), would be great. Currently there are 2 ways to know from which struct, a method was coming:

• parsing the name field of the function: it is not accurate and source of unmaintainable code.
• looping into the arguments field to find a parameter named self: it is really inefficient. You can argue that just checking the first argument of the arguments list is enough but I do not think that it is a robust solution. The arguments field is intended to describe arguments list of the function. So even if it will never happen, if tomorrow you decide that a better place for the self parameter is the last place in the list, my code is broken (or maybe more probable: you can decide that this is a better name than self). Whatever the change you decide to make, you are in your right to do this because the intention behind the arguments list is to describe arguments list of the function. Not giving the struct name from where the method is originally coming. So I really think that adding a dedicated field is not redundant.

Thank you.

[ShironekoBen]

Yeah, self being non-nullable seems like a very straightforward change that should definitely be added. As for pointers being array pointers or not, I don't mind adding a "speculative" flag for that (essentially only defined on self for the time being), although I'm not entirely sure what to call it. arity springs to mind, with arity: "single" or arity: "multiple" representing the not-an-array case and the definitely-an-array cases respectively (with no arity key indicating "we don't know"). Does that sound sensible?

As for is_method_of... hmm, the "best" way to get that data at the moment is to parse original_fully_qualified_name, as that is less ambiguous than name, but you still can't tell the difference between namespaces and classes/structs. I could add a parameter that indicated the name of the owning class IFF it is a class/struct, I guess? That's not a particularly hard thing to do, but I have a suspicion it might not be exactly what you want as it will flag static methods as well since they are technically class members.

So my suggestion would probably be something two-fold - add a original_owning_struct field that indicates which struct (if any) originally owned this function, and then also add an is_instance_pointer field to arguments that indicates if the argument in question is supposed to be the this pointer (incidentally, the reason the parameter name is self is because this causes problems in languages where this is treated as a keyword - hence that particular change is pretty unlikely!). Thus the logic becomes something like:

1) If original_owning_struct is not present, this is a loose function (potentially in a namespace - parse original_fully_qualified_name if you need the name). 2) Otherwise this is a member of original_owning_struct 3) Check the arguments list to see if any argument has is_instance_pointer set to true. If so then it is a member function and needs a this pointer passed as that argument. 4) Otherwise this is a static function.

How does that sound? It does involve looping through the arguments list, true, but at the same time I think anything generating bindings that cares about struct membership is also likely trying to "hide" that by wrapping the API in a struct-like form in their target language, and thus by extension they need to know precisely which parameter this is so that they can generate code to pass their instance pointer as that argument, so that doesn't seem avoidable.

[ShironekoBen]

Oh, as for PRs - I'm always open to PRs, but FWIW these don't seem like massively complicated changes to get in and I'm currently in a rare state of having a few days holiday so I can probably get them implemented relatively quickly if the spec seems like it makes sense.

[tiawl]

I'm not entirely sure what to call it. arity springs to mind

Yeah, arity seems well-suited for what it is describing.

I could add a parameter that indicated the name of the owning class IFF it is a class/struct, I guess? That's not a particularly hard thing to do, but I have a suspicion it might not be exactly what you want as it will flag static methods as well since they are technically class members.

Currently for my use case, it is enough. I do not mind to make the distinction between static functions and methods. However for potential other users make the distinction should be interesting.

How does that sound?

It sounds great, thank you for the answer. I will close this issue with your next update.

[ShironekoBen]

OK, I've added some of these changes. Specifically:

• self arguments now set is_nullable to false
• Functions have a original_class member that gives the class name they were originally declared on (this already existed internally with that name, hence it being original_class not original_struct as discussed above)
• The self function argument now sets is_instance_pointer to true so it can be easily identified

With regards to arity, when I started looking at that I realised that it's not quite as simple to add as I had thought - internally there's a step where the type data gets converted to a "C-like" string representation when being passed between the DOM and the type comprehension engine. At that point non-nullable pointers are flagged by using the special symbol ^ instead of *, but to carry arity information across that boundary would require coming up with an encoding for it in string format and then extending the type comprehension parser to understand that.

That isn't horrifically difficult, but given that right now the only case we have defined arity for is the self argument, and that can already be identified with is_instance_pointer, it seemed like it would be a fair bit of work for relatively little benefit at the moment, and potentially even something that would need to be refactored again anyway if/when a more broadly applicable version of arity got implemented.

So for now I've left it alone, on the assumption that for now is_instance_pointer provides enough information for the current use-case. Let me know if that isn't true and I can give it more thought if necessary!

[tiawl]

Thank you for your work, I am going to test that during the next days. If something goes wrong I am going to open an other issue.