On Wed, 2013-02-13 at 00:30 +0100, Richard Braun wrote: > On Wed, Feb 13, 2013 at 12:12:57AM +0100, Svante Signell wrote: > > What a pity, I spent a very long time doing this rewrite, and to make > > the code more readable (at least in my opinion). Back to spaghetti^Wold > > style code then. > > We warned you several times about making your changes as little > intrusive as possible for easier review. Since you seem to be unable to > understand what that implies, it's only natural Samuel no longer wastes > time on this and simply replies "to long to go through it".
I am able to understand, no problem. And I know that a patch this large can be difficult to review. Problem is that he did not give any hints on _how_ to make the rewrite. If introducing the SELECT_ERROR had been discussed (or allowed to introduce) my solution would have been different too. But I would still use the helper functions to improve readability (and maintainability). > You have to understand that whether the code looks "spaghetti" or not, > your goal here is not to fix that, it's to fix a POSIX conformance > issue. If you want to refactor, fine, but please separate it. It's > really important to make as little change as possible for two reasons. > The first is the obvious comfort for the reviewers. Agreed! > If it's easier for > us, it's also faster, and we can more effectively concentrate on > thinking about the big picture and the details. The second is to reduce > the likeliness of regressions. Please keep that in mind. Yes, the risk of regressions increase proportionally to the number of changes. But sometimes, introducing fixes for every corner case in an old design can create other bugs and/or side effects. It's all a matter of hierarchical versus flat design methodology. The second approach fails completely at some stage when the total complexity increases. Then starting from scratch is the way to go (perhaps hurdselect.c is not of the second kind, though).
