On Fri, May 16, 2025 at 12:27:29PM -0400, Derrick Stolee wrote: > > I imagine that "start" here is setup for the following commit which will > > parallelize this task across multiple threads, with each thread starting > > at a different position. > > > > I wonder if (as an alternative) we could get away with passing in a > > (struct packing_region *, size_t) pair and drop "start". I think this > > can work if you pass in the result of adding whatever the value of > > "start" would be to to_pack.regions here. > > > > That somewhat hides the fact that this code is meant to be run across > > multiple threads, but in a way that I think is worth doing. It lets the > > function avoid having to do things like: > > > > for (size_t i = start; i < start + nr; i++) > > > > and instead do the simpler: > > > > for (size_t i = 0; i < nr; i++) > > > > since the caller already adjusted the regions pointer for us. As a > > side-effect, it also means that the call below in prepare_pack() can > > avoid passing a literal zero. > > I generally avoid this kind of pattern, because it makes it more > difficult to understand what is going on when debugging. The cost > of adding 'start' to the index seems low relative to having a > consistent indexing scheme when accessing from the "global" list > of items. > > This only gets more complicated when the threaded version iterates > on a "consecutive sublist of regions". That's fair. After thinking about it some more, I agree that enumerating [start, start + nr) is clearer than enumerating [0, nr) and adjusting the offset at each invocation of the loop body. Funny enough, I applied that suggestion in a couple of spots in one of the incremental MIDX patches I sent recently. > > This is an extreme nitpick, but I think that "/**" (as opposed to "/*") > > is preferred, as the former is used for Doxygen-style comments. I feel > > like I have seen Junio comment on this before, but searching 'f:Junio > > "/**"' yields a measly 83,000+ results, so I am unlikely to find it ;-). > > > > (Not worth rerolling on its own, but if you are rerolling anyway, which > > is what I gathered from some of your earlier replies, it may be worth > > picking up along the way.) > > I'm confused. Are you saying I should change from "/**" to "/*" (which > is the opposite of your preferred statement) or did you misread that I > had used "/**"? I can't determine which statement is the intended one. Oops, this is a typo on my part. I was saying that we should prefer "/*" to "/**". The "/**"-style lives on in v4, but this is *extremely* minor and I don't think is worth sending another round when everything (of what I have read so far from v4) looks good to me. Thanks, Taylor
