On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote: > This probably isn't worth it, but I wondered if this wouldn't be easier > if we pulled that memory management into the caller, it's not > performance sensitive (or maybe, how many alternatives do people have > :)), but an advantage of this is that we avoid the free()/malloc() if we > only get partway through, i.e. return early and keep looping. I agree with your "probably". This isn't worth it to save a malloc in a very non-hot code path. If the two bits of code were otherwise equal (say, reset-ing a buffer used directly in a loop) I might say "why not?". But crossing a function boundary to me introduces way too many questions in somebody reading the code (like "is pathbuf supposed to have something in it?") to make it worth doing here. But even if we did want to do it, see below. > In terms of general code smell & how we manage the "return" here, as > adding "RESULT_MUST_BE_USED" to this shows we never use the "0" or "-1" > (or any other...) return value. > > That's been the case since this was added in c2f493a4ae1 (Transitively > read alternatives, 2006-05-07), so we can probably just make this a > "void" and ditch the returns if we're finding ourselves juggling these > return values... Yeah, we could ditch the return values. In a sense they are at least documenting how link_alt_odb_entry() sees the world, but if nobody looks at them, I'd be OK dropping them to make it clear that we don't intend to ever act on them. That said, both of these are orthogonal to what Glen's patches are doing. If you want to submit a series later to deal with them, OK, but let's try not to hijack the conversation for patches that are fixing a real bug. -Peff
