Re: [PATCH 18/20] pack-revindex: remove unused 'find_revindex_position()'
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 11, 2021, 17:11 UTC
- Message-ID
- <61f6acde-3788-03ee-8dce-f621984a3402@gmail.com>
- In-Reply-To
- <X/x8dXFfQUdpKeVn@nand.local>
On 1/11/2021 11:27 AM, Taylor Blau wrote:
Show 10 quoted lines
> 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.
Show 9 quoted lines
> 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