On Tue, Feb 27, 2024 at 1:42 PM Ian Rogers <irogers@xxxxxxxxxx> wrote: > > On Tue, Feb 27, 2024 at 11:17 AM Arnaldo Carvalho de Melo > <acme@xxxxxxxxxx> wrote: > > > > On Tue, Feb 27, 2024 at 09:31:33AM -0800, Namhyung Kim wrote: > > > I can see some other differences like machine__findnew_thread() > > > which I think is due to the locking change. Maybe we can fix the > > > problem before moving the code and let the code move simple. > > > > I was going to suggest that, agreed. > > > > We may start doing a refactoring, then find a bug, at that point we > > first fix the problem them go back to refactoring. > > Sure I do this all the time. Your typical complaint on the v+1 patch > set is to move the bug fixes to the front of the changes. On the v+2 > patch set the bug fixes get applied but not the rest of the patch > series, etc. > > Here we are refactoring code for an rb-tree implementation of threads > and worrying about its correctness. There's no indication it's not > correct, it is largely copy and paste, there is also good evidence in > the locking disciple it is more correct. The next patch deletes that > implementation, replacing it with a hash table. Were I not trying to > break things apart I could squash those 2 patches together, but I've > tried to do the right thing. Now we're trying to micro correct, break > apart, etc. a state that gets deleted. A reviewer could equally > criticise this being 2 changes rather than 1, and the cognitive load > of having to look at code that gets deleted. At some point it is a > judgement call, and I think this patch is actually the right size. I > think what is missing here is some motivation in the commit message to > the findnew refactoring and so I'll add that. I'm not against your approach and actually appreciate your effort to split rb-tree refactoring and hash table introduction. What I'm asking is just to separate out the code moving. I think you can do whatever you want in the current file. Once you have the final code you can move it to its own file exactly the same. When I look at this commit, say a few years later, I won't expect a commit that says moving something to a new file has other changes. Thanks, Namhyung
