Re: [PATCH 18/20] pack-revindex: remove unused 'find_revindex_position()'

[Derrick Stolee <stolee@gmail.com>]

On 1/11/2021 11:27 AM, Taylor Blau wrote:
> On Mon, Jan 11, 2021 at 06:57:00AM -0500, Derrick Stolee wrote:
>> Not that this is new to the current patch, but this patch made me
>> wonder if we should initialize *pos = -1 in the case of a failure
>> to find the position? A correct caller should not use the value
>> if they are checking for the fail-to-find case properly. But, I
>> could see someone making a mistake and having trouble diagnosing
>> the problem because their position variable was initialized to
>> zero or a previous successful case.
> 
> *pos = -1 may be more confusing than clarifying since pos is unsigned.

RIGHT. My bad.

> It would be nice if there was a clear signal beyond returning a negative
> value. I guess you could take a double pointer here which would allow
> you to assign NULL, but that feels rather cumbersome as a means to catch
> callers who failed to check the return value.
> 
> It does raise the argument of whether or not we should allow the
> program to continue at all if 'ret < 0' (i.e., 'offset_to_pack_pos()'
> either 'die()'s or returns a usable uint32_t), but I'm OK with the
> current behavior.

I was thinking "*pos = -1" was a free way to "help" a developer
who uses the API incorrectly, but it's _not_ free. Ignore me.

Thanks,
-Stolee