On Fri, Oct 14 2022, Phillip Wood wrote: > Hi Ævar Hi, thanks for taking a look again. > On 12/10/2022 22:02, Ævar Arnfjörð Bjarmason wrote: >> For a general overview see the v2 CL: >> https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@xxxxxxxxx/ >> Changes since v2: >> * Ejected various not-to-the-point of converting to the "opts >> struct" >> per feedback about attempting to make this leaner. > > We're back to the same number of patches as v1 but you've removed > three test cleanups and squashed three patches together which means > there are five new patches in this version - what are they doing? There's nothing *new* in this version, but per the range-diff of v2 some of the new ones in that version were kept here. I think that's what you're taling about. Mainly it's the 'Only the "ungroup" was here in v1[...]' part of the v2 CL: 1. https://lore.kernel.org/git/cover-v2-00.22-00000000000-20221012T084850Z-avarab@xxxxxxxxx/ >> * I kept the size_t change & the online_cpus() fallback, and updated >> a commit message for the latter. For "int" v.s. "size_t" once we're >> not handling "-1" to mean "use the default" it makes sense to be >> unsigned, and if we're re-doing that at this point we'd need >> rewrites for "!processes" assumptions. > > I left some comments about this, I think the size_t change is taking > us in the wrong direction as it is introducing a number of new > implicit signed->unsigned conversions. I'm still not sure why you need > the online_cups() changes c.f. > https://lore.kernel.org/git/8f95fbdb-b211-56af-8693-0e5a84afebac@xxxxxxxxx/ > which has never had a reply Sorry about the non-reply there, between that & later discussion I tried to address that in the "Keeping this default behavior just for[...]" commit message update in this v3 (see range-diff). But no, those changes are not strictly needed. But it's a trade-off I decided to take. In the v1 (linked above) you pointed out that we could simply copy this field to the "struct pp" (a shorthand for "struct parallel_processes", I assume). That's true, but for maintaining & understanding this API I think it's much easier to reason about it when all of our user options are "const", and we don't second guess any of those, and the "struct parallel_processes" For the v1 I can see what that was easy to miss, as we still kept the copy of the number of processes in the "struct parallel_processes". In the v2 and this v3 we get to the point where we can remove that, and "ungroup", the copies of the callbacks etc. So, leaving out the "provide a default" seemed worth it in the end, it's just 4 additional lines in the callers per the 05/15 (most of them had those already). You also had a related concern in 04/05 (which I'm taking the liberty of replying ot here): https://lore.kernel.org/git/a7463bc5-9a92-8f0f-c0ee-e72fbbeedc09@xxxxxxxxxxxxx/ So, first I disagree with it "going in the wrong direction". We've been converting more things to size_t. For e.g. an "int nr_processes" we can expect that we'll want to e.g. have a corresponding "struct string_list" whose "nr" is a "size_t" (or similar aggregate types). By mixing the two we're mixing types forever with associated warnings (& suppressions). We've been changing these sort of things to a "size_t" elsewhere, e.g. dd38e9e510c (trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctx, 2022-10-12) is one recent example.. But yes, we do incur warnings under DEVOPTS=extra-all now because things outside of the API are still "int" in some cases, just as we do with e.g. "struct string_list" and its users. As to your: Before this patch we could have caught a negative value of n with an assertion. After this patch a negative value passed will become a large positive value (you have audited the current callers but that does not protect us from future mistakes). I that's a good point. In this case I thought the likelihood that anyone would accidentally pass ".processes = -1" or whatever wasn't worth worrying about. If you think it's worth worrying about I think that concern would be easily addressed e.g. with: if (n == SIZE_MAX) BUG("overflow?"); Or whatever. A much likelier edge case IMO is that you'd have some pattern where you init a "processes" with "{ 0 }" or whatever, so it's zero, and then we'd interpret that zero as online_cpus(), which e.g. for the in-flight paralellizing of certain "git submodule" operations is probably too high a number. Since we don't interpret !n or n < 1 as online_cpus() anymore we can BUG() out on it, which I think is an improvement. The 4 lines of additions to the relevant callers to call online_cpus() themselves are better than having those or a new caller potentially have this DWYMery from the low-level API. >> * Squashed the multi-step introduction of the "opts" struct, per >> Phillip's suggestion. > That's most welcome, thanks Glad you like it!
