On Thu, Dec 26, 2019 at 8:35 PM Nicolas George <geo...@nsup.org> wrote:
> Andreas Rheinhardt (12019-12-26): > > b) It guarantees to not allocated more than UINT_MAX - 1 elements, so > > the caller needn't check for overflow if the desired size is increased > > in steps of one. > > This is preparing trouble for later, I don't understand. Do you think that callers will take this as a blank cheque to not check at all? > and as Michael pointed, it will not > work when the number of elements is stored in a signed integer. My intention was actually to convert the types for every user of this function when using this function (see e.g. the -Matroska demuxer patch). > (It was > a mistake to use a signed integer in the first place, but that > particular mistake is already all over the place). I agree. > I strongly suggest we > try to make things properly: > > - Make nb_allocated and min_nb size_t as they should be. > Do we have arrays where there is a need to go beyond the range of ordinary integers for the number of elements? > > - Do not hard-code UINT_MAX-1, make it an argument, max_nb: that way, if > the index is an integer, just pass INT_MAX-1, and if it is properly > size_t, pass SIZE_MAX-1. > > - Since we can't pass a pointer to int as a pointer to size_t, add a > small convenience wrapper av_fast_realloc_array_int() just to convert > the type of nb_allocated. That's ugly, but I do not see a better > solution. Or we can decide we must change the type of the size to > size_t whenever we update code to use av_fast_realloc_array(). > I'd like not to turn the int/unsigned version of av_fast_realloc_array() into a wrapper until there is a real case where an array is needed that doesn't fit into an int (or an unsigned). And switching to size_t everywhere would increase the size of certain structures which I very much like to avoid (e.g. the size of every MatroskaIndex element would increase by eight bytes (for 64 bit systems)). > > And while we are at it, I would like better: > > - Alter ptr the same way nb_allocated is altered, and return a real > error code in case of error. Otherwise, the caller has to guess that > only AVERROR(ENOMEM) is possible, and we have seen that these > assumptions and hard-coded error code have a way of overstaying their > welcome. > That is certainly possible (if "alter ptr the same way nb_allocated is altered" means: Via a pointer to a pointer. (The other interpretation (that the array should be automatically freed on failure and ptr set to NULL) would have elicited a different response.)) > > Poor choice of types have caused problems in the past. They will cause > more problems as the memory of computers, and therefore the task we give > them, grow. Let us not cultivate them. > > Regards, > > -- > Nicolas George > Thanks for looking over this. - Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
