Add accessor functions for the callback data structs from the C API

[cyderize]

It doesn't seem possible to use callback data in a way that allows for swapping out HiGHS DLLs for newer versions safely.

In 1.7.0, it looks like a new pdlp_iteration_count member was added to the HighsCallbackDataOut struct (see here). This moves all subsequent fields, so we can't access the data in 1.7.0 using our application built for HiGHS 1.6.0 (and now to make it compatible with both, we'd have to check the HiGHS version and cast to the different structs).

Generally, I think the way to deal with this would be to add accessor functions to the C API for getting the values of the struct members. That way even if the layout of the struct changes, the accessor functions will still work.

[jajhall]

Sorry about this: I didn't think sufficiently when I introduced this value to HighsInfo.

I don't have the time to write accessor functions - whatever they are - and we need to get 1.7.1 out as soon as possible. Maybe you could do this and open a PR.

Alternatively, I could move the new data member to the end of the struct until HiGHS 2.0.

Any thoughts @odow?

[cyderize]

I think it might be a good idea to move the new data member to the end anyway because that will mean that applications written for 1.6.0 will again work for later versions (other than 1.7.0), and then if new future members are always only added to the end, that's probably good enough (since the structs are already part of the API, even with new accessor functions, you wouldn't be able to put new members anywhere else in a backwards compatible way since older users would still be using the structs directly).

Then I guess in 2.0 the accessor functions could be added and made the sole way to access the members so that the internal structs could be changed whenever needed.

I'm happy to open a PR to add the accessor functions, but I won't have time until the week after next week (but in any case I don't think that needs to be in 1.7.1 anyway - moving the new member to the end would already fix the problem).

[jajhall]

So, I've moved pdlp_iteration_count to last data member of HighsCallbackDataOut.

If I understand correctly, inserting pdlp_iteration_count into HighsInfoStruct is OK because the C API has Highs_getIntInfoValue (etc) to get info values. Is this what you mean by an "accessor function"?

I'm still confused why the position of pdlp_iteration_count matters, if you're using the HighsCallbackDataOut struct in C. Don't you have an instance "foo" of the struct and access foo.objective_function_value (say), rather than depending on objective_function_value being the 4th member of the struct (as it was in 1.6.3)?

I was confusing access to structs with the need for hard-coded ints in the C API that are needed to access members of C++ enum classes

[jajhall]

Closed by #1698

[cyderize]

Yes actually, I think you're right - there already are accessor functions for these, and they don't need the callback data struct at all, so it will all work if I just use Highs_getIntInfoValue etc. Thanks, for some reason I didn't think of that, but that ought to be the right solution for us.

The reason why the position matters for us is if that we're dynamically loading HiGHS as a DLL at runtime - so the user can expect it to be possible to replace the HiGHS 1.6.0 DLL with a HiGHS 1.7.0 DLL and for it to still work since it should be backwards compatible.

Basically, our program was compiled using the the 1.6.0 header, so the C compiler assumes that foo.objective_function_value is at position 5 in the struct, if the user then replaces the 1.6.0 DLL file with the 1.7.0 DLL, that's no longer correct. So we'd have to recompile our program with the 1.7.0 headers to make it work.

So I suppose it's a question of whether we should expect ABI compatibility here for the C API - or whether it's only guaranteed that you can update to a newer version of HiGHS by also updating the headers and recompiling.

[jajhall]

I'm not a C programmer, so it's good to know that the struct isn't enough. I guess you might be using offsets from a pointer to it.

You can't call any HiGHS methods in the callback. For example the HighsInfo data member you want won't have been populated when the callback is called.

At some point I'll put something to return an error status if any HiGHS method is called

[odow]

I guess you might be using offsets from a pointer to it

Yes, precisely.

This issue is what I flagged in jump-dev/HiGHS.jl#205 (comment)

[cyderize]

I'm not a C programmer, so it's good to know that the struct isn't enough. I guess you might be using offsets from a pointer to it.

You can't call any HiGHS methods in the callback. For example the HighsInfo data member you want won't have been populated when the callback is called.

At some point I'll put something to return an error status if any HiGHS method is called

Ah yes, the highs methods don't work inside the callback - darn, I thought that would be the perfect solution to make things work for every version.

Thanks for the quick fix of moving it to the end of the struct!

I suppose in summary, for a C API, there are basically two ways of dealing with structs while avoiding ABI breakage:

• Ensure that new members only get added to the end of them, or
• Don't expose the struct at all, and instead give an opaque void* pointer to users and add functions to access each member using that opaque pointer (this is what I meant by accessor functions).

The second option has the advantage of letting you move around stuff in the internal struct without breaking binary compatibility in the C API, if that's something that's useful, but if you don't mind just always adding things to the end of the struct, the first option will work just fine.

Thanks to all for the quick responses to everything.

[jajhall]

Accessor functions for the callback struct members would be possible, as they access populated data members.

What I need to handle with an immediate error return are calls to most methods in the HiGHS class

[jajhall]

Keeping this open as a reminder to write the access functions

[jajhall]

An accessor function Highs_getCallbackDataOutItem for the C API has been written, but there is a strange set of CI failures in #1748.

It seemed like a good idea to introduce const char* definitions such as kHighsCallbackDataOutLogTypeName in highs_c_api.h so users pass these rather than explicit strings as the name argument, but mingw claims the following (and for all other names)

multiple definition of `kHighsCallbackDataOutLogTypeName'

Strange, since there are other constants defined in highs_c_api.h without complaint.

[jajhall]

It seemed like a good idea to introduce const char* definitions such as kHighsCallbackDataOutLogTypeName in highs_c_api.h so users pass these rather than explicit strings as the name argument, but mingw claims the following (and for all other names)

multiple definition of `kHighsCallbackDataOutLogTypeName'

Strange, since there are other constants defined in highs_c_api.h without complaint.

Sidestepped this for now

[jajhall]

@cyderize Please would you let me know if what's in #1748 is what you had in mind.

It would also be great to understand why the const char* definitions in highs_c_api.h, such as kHighsCallbackDataOutLogTypeName, aren't accepted

[cyderize]

@cyderize Please would you let me know if what's in #1748 is what you had in mind.

It would also be great to understand why the const char* definitions in highs_c_api.h, such as kHighsCallbackDataOutLogTypeName, aren't accepted

Yes, this looks like it'll work - I do wonder if it's better to just have separate functions for each data member, rather than using the string names (that way the signature can just have the correct return type instead of having to use void*) but maybe this is more consistent with how the other similar functions work.

I think if you use const char* const for the definitions they should work. The problem is that const char* variables get 'external linkage', which means if you include the header more than one c/cpp file, they both try to create the same external symbol and conflict.

The const in the const int declarations is what makes those work (const variables get internal linkage and don't conflict), it's just that annoyingly for pointers like const char* you need the pointer itself to be const (as opposed to the data being pointed to) - so you need const after the * as well.

[jajhall]

Many thanks @cyderize , I'll keep the single method and passing names for consistency with other such methods. Thanks for the 'const char* const' tip: I've not grown up writing C/C++, so would never have known.

[jajhall]

Closed by #1748