Fixed llama_set_state_data declaration not matching definition

[niansa]

Nothing to comment here.

[ggerganov]

Probably looking at something outdated - I changed the definition today:

https://github.com/ggerganov/llama.cpp/blob/affc76edfdefa7b326f526e463cc65ff13fcfb92/llama.cpp#L2676

[niansa]

@ggerganov actually nope, it's still in master like that
https://github.com/ggerganov/llama.cpp/blob/master/llama.h#L154

[niansa]

Seems like you've reverted your fix right after you've commited it...

[ggerganov]

I changed const uint8_t * src to uint8_t * src on purpose (both declaration and definition), because in the implementation we assign it to void * which used to generate a compile warning about not being const correct:

8a203f9

[niansa]

Hmm, not sure if it's better to require the API user to do a const_cast rather doing that internally...

[imaami]

I changed const uint8_t * src to uint8_t * src on purpose (both declaration and definition), because in the implementation we assign it to void * which used to generate a compile warning about not being const correct:

8a203f9

Is it possible that the source buffer is modified during llama_set_state_data()? If so, then non-const is the correct type, but in that case the API should clearly document that the input data may be modified.

Looks like the cast to void * is done when assigning the pointer to ggml_tensor::data, which is void *. Is there a reason why that member can't be void const *data? Is it sometimes used to store the address of memory that gets modified? If that's the case then it seems a tad problematic in itself.

Requiring the caller to const_cast a potentially read-only buffer to non-const is sus, and the alternative of making a temporary copy every time isn't that great either. No matter how you put it it's just wrong to cast towards less const-ness, even if it happens inside the implementation (like casting a potentially read-only data pointer to void *).

If ggml_tensor::data can't be made void const * then a C-style hacky solution would be replacing it with

    union {
        void *data;
        void const *cdata;
    };

and changing the implementation to only use one or the other union member depending on the pointer value's origin. Of course this only makes sense if the correct choice is obvious from the context alone, otherwise it would need a runtime tag member, in which case it might just be better to have two separate pointers.