On Sun, 10 May 2015, Segher Boessenkool wrote: > > This clearly renames rather than removing the `rlwinm' pattern, please > > correctly reflect that in ChangeLog. Some other, unnamed patterns are > > given names rather than deleted as well, just as you've noted at the top. > > And none of the other changes are mentioned in your ChangeLog entry. > > The changelog says it deletes the patterns with the old names. Which it > does. And it adds the ones with the new names. Which it does. No one > said changelogs are useful ;-)
No one? Well, I'm saying it now, then. :) > > Would you be able to split this change up further by any chance? > > I could, but it would be a lot of extra work. First, I haven't done those > steps in order (they aren't "steps" at all): I take one pattern, and fix > it up, then the next, etc. Yeah, I know the pain, it's usually how it happens. You start cleaning up things and then you find yourself having done a number conceptually independent changes that overlap one another in the source modified. I've been through it many times. The thing is this extra work of untangling is worth doing as it'll help the maintainer and other members of the community, including those investigating change history months if not years from now for one reason or another, understand what the consequences of conceptually individual changes are. Specifically which changes are obviously harmless (e.g. formatting changes or the addition of debug `*' pattern names) and which are potential trouble makers (e.g. the reordering of operands or folding `define_insn' and `define_split' into `define_insn_and_split'), that e.g. need to be taken into account when porting changes that may rely on them. When all the bits are lumped together, it's very difficult to decipher them and easy to miss something. So I've done such patch splitting and rearrangement many times, using various techniques. For example it's often faster and easier if you hand-edit the patch being split into two rather than trying to reproduce the intermediate stage in the source being patched. Other people may have other hints and tricks. Of course you'll want to keep the original final version of the file changed around and then you can compare it to the result of applying a patch series, to make sure both results are identical. > But much worse: if you do tiny pattern changes like you suggest, I can > guarantee you the patches _will_ mis-apply. All of the time :-( Well, if you get the line numbers recorded in patches right, which is how `diff' generates them, then it won't happen. And as a committer you can verify you applied your own patches correctly, by taking a diff against your original patched version before pushing the changes into the repo. Especially when other upstream source changes have since caused line numbers to shift. > > change addresses a single issue only. That would avoid problems with > > ChangeLog and make the review easier. > > This isn't the first patch like this, I've cleaned up most other PowerPC > integer patterns already. It's enough work already. Often the changes > cannot be separated, and even if they can you then need them in the fixed > order you made for them, etc. Of course changes can be separated (if they cannot, then they weren't really conceptually separate in the first place), and you do need to get the order right. E.g. patches that are potentially of interest to older release branches need to go first, followed by ones that are meant to stay on trunk only. I'll leave it up to David to decide anyway as he's the port maintainer, and I'm only a member of the community who happened to poke at this port once or twice in the past. Maciej
