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

On 1/11/2021 11:27 AM, Taylor Blau wrote:

On Mon, Jan 11, 2021 at 06:57:00AM -0500, Derrick Stolee wrote:

quoted

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