`json+struct` codec should take pointers to pointers to **non-const** buffers

[bhaller]

Making a new issue for this at the request of @jeromekelleher since #3306 was merged and closed. Tagging @petrelharp since he'll be a client of this API too, in pyslim.

Copy-paste from #3306:

In the declaration:

tsk_json_struct_metadata_get_blob(const char *metadata, tsk_size_t metadata_length, const char **json, tsk_size_t *json_length, const uint8_t **blob, tsk_size_t *blob_length)
can const uint8_t **blob be uint8_t **blob without the const? SLiM sometimes actually munges the incoming mutation metadata (remapping population indices if the user has asked for that, prior to the main metadata-parsing code working with the data). I could of course make a copy of the binary blob, but since it is expected to be quite large I'd prefer not to, so as to not push the memory high-water mark unnecessarily. It doesn't seem like tskit has any special interest in keeping that metadata const, right? I'd have to cast away the const to modify it, though, which according to the C++ standard is "undefined behavior" although I imagine it would probably work in practice.

For consistency the JSON data should perhaps also not be const, but I don't care about that one way or the other; SLiM doesn't need to modify that, and if it did, I could just copy the data since that is small.

(If you want the mainstream API to be const for safety, perhaps you could provide a non-const version of it for those who need it?)

In followup discussion, @jeromekelleher wrote:

These have been changes to const char **blob, for consistency. The const is necessary here for us to compile and signals that the function doesn't alter the buffers.

I replied (edited/expanded a bit):

It's a bit annoying to require a caller to pass a pointer to const just because the function itself doesn't alter the buffers. If it was just a question of a const char * parameter and the user passing in a char * to the function, that would be fine; the compiler would automatically accept the char * for the parameter, rather than requiring the caller to provide a const char *. But because of the extra level of indirection, a const char ** parameter does not automatically accept a char ** parameter, requiring the caller to do some faffing about with casts and such. I would argue that it is not standard practice to put the const there – when passing a pointer to a pointer to data, it is already assumed that the function is not supposed to muck around inside the data that is ultimately pointed to, and that guarantee is not typically made with const in APIs (and in many cases cannot be made, in C/C++ syntax, such as guaranteeing that a function won't modify stuff that is pointed to by pointers contained within a const structure that it is passed in; C and C++ don't have any concept of "deep" constness). So I don't think you ought to have that const there. But it's not the end of the world; I thought initially that dealing with this API issue would require SLiM to cast away const, producing undefined behavior, but that is not the case. It's just annoying and non-standard (IMHO).

(I'd question the decision to use char instead of uint8_t, though; they are not the same thing, and I think the semantics of uint8_t are more appropriate – for the binary data, at least.)

[jeromekelleher]

We're using char* as the opaque data pointer consistently throughout tskit APIs afaik, so there would need to be a compelling reason for uint8 here to go against that convention.

Regarding the const char** argument I'll need to review the apis for usage to see what our existing conventions are. If you'd like to make things happen more quickly then you could have a look.

The basic principle here is consistency is king - even if it's slightly wrong or against someone's aesthetics, it's better to be consistent overall when adding to a large and stable API.

[bhaller]

We're using char* as the opaque data pointer consistently throughout tskit APIs afaik, so there would need to be a compelling reason for uint8 here to go against that convention.

OK.

Regarding the const char** argument I'll need to review the apis for usage to see what our existing conventions are. If you'd like to make things happen more quickly then you could have a look.

OK, I did a quick regex search for "\* *\* *[_a-zA-Z]+.*". In summary, the most parallel case is (I think?) in kastore, in the kastore_get() family of functions; they do not use const, and use void for the opaque data pointer (not char or uint8_t). Hits in tskit itself sometimes use const and sometimes don't; I'm not terribly familiar with these APIs, but I think the const is left off in some cases where the function in question does not modify the data pointed to, suggesting that the const could also be left off for tsk_json_struct_metadata_get_blob(). Mostly it seems like const is used in cases where neither the function nor the caller want to modify the data pointed to – which is not the situation for this json+struct API.

Here are the details on the hits I found, if you want to peruse them:

It looks like in kastore const is not used for what I think is maybe a similar case, e.g.:

int kastore_gets_int8(
    kastore_t *self, const char *key, int8_t **array, size_t *array_len);

and similar. (It looks like kastore uses void ** rather than char ** for its opaque data pointer, it looks like, as in kastore_get()).

In core.h, __tsk_safe_free() uses void ** not char **; I don't see other ** cases there.

In genotypes.h tsk_variant_init() uses const char **alleles, but that is a situation where the caller presumably has no interest in modifying the data, so const is fine there – it's const for everybody, so it isn't comparable.

In tables.h, we've got:

int tsk_identity_segments_get_items(const tsk_identity_segments_t *self, tsk_id_t *pairs,
    tsk_identity_segment_list_t **lists);
int tsk_identity_segments_get(const tsk_identity_segments_t *self, tsk_id_t a,
    tsk_id_t b, tsk_identity_segment_list_t **ret_list);

I'm not sure what those are doing exactly, but they don't use const for their ** parameter. No other hits there.

In trees.h the relevant hits are:

int tsk_tree_get_sites(
    const tsk_tree_t *self, const tsk_site_t **sites, tsk_size_t *sites_length);

and

int tsk_tree_map_mutations(tsk_tree_t *self, int32_t *genotypes, double *cost_matrix,
    tsk_flags_t options, int32_t *ancestral_state, tsk_size_t *num_transitions,
    tsk_state_transition_t **transitions);

The first one uses const, but that is (I think?) again a "const for everybody" situation; the caller presumably has no desire to have a non-const pointer to that data. The second one does not use const; I'm not familiar with that call, so I'm not sure what's going on there (i.e., whether that function modifies the data pointed to or not).

So that's what I found, but it might require someone with better knowledge of tskit than myself to interpret it correctly.

The basic principle here is consistency is king - even if it's slightly wrong or against someone's aesthetics, it's better to be consistent overall when adding to a large and stable API.

OK. As I wrote above, I thought initially that with const on this API I would be required to do an unsafe cast with undefined behavior (which would really be unacceptable), but that turns out to be untrue; what is at stake is just a bit of gross/confusing casting hoopla for the caller. I don't think const should be on this API, but I am happy to bow to consistency as the overriding principle – if consistency can be found in the above survey. :->

[jeromekelleher]

Great, thanks @bhaller - let's drop the const so. Would you like to make the PR? I think it would be a good exercise at this point given the available C expertise for tskit.

[bhaller]

Great, thanks @bhaller - let's drop the const so.

Great.

Would you like to make the PR? I think it would be a good exercise at this point given the available C expertise for tskit.

I'm not the right person to do this, no; I have no idea how to work inside the tskit code base, and there are Python repercussions and tests involved that I don't have any idea about. It should be a trivial change, presumably? If you guys are too busy to do it, perhaps @petrelharp can take this on.

[petrelharp]

I got it