On Tue, Sep 16, 2014 at 01:03:50PM +0000, Joseph S. Myers wrote: > The point of testsuite additions is to verify the visible changes in
Understood. A long time ago I worked on mil-spec systems.. > > Come to think of it, what if I decline to make any testsuite > > additions? > > Then the patch isn't ready for review. Documentation and testcases are > the first thing I look at when reviewing C front-end changes; the > testcases are the primary evidence that the patch does what it's meant to > do, and without them I won't generally try to review the code changes. I gave you two testcases. I happen to know how nice it is to have testcases when reviewing patches. Yes, they weren't testsuite patches. Now I'd normally just grumble a bit to myself and comply with a request for a testsuite patch, but I'm a little annoyed. The testsuite isn't my personal itch. I could have just bisected until finding Jan's patch, and asked him nicely to please fix his breakage. This is what most people do, and pointing the finger at a particular patch is useful. I went further and analysed exactly why the patch was at fault. I could have stopped there. The breakage wasn't really affecting me, and it wasn't my bug after all. Instead I thought I'd have a go at fixing the problem, without bothering Jan: The loss of section attribute looked quite easy to fix. So I threw together a patch, verified it worked, then knowing that there is more than one merge_decls in the gcc source code, fixed the same problem for C++. At that point I found Andrew's bugzilla. Andrew is competent, I could have just attached the patch I had to his bugzilla and left him to it. However it occurred to me that Jan's changes might have broken something else, so I poked around and found some other fields that had been moved to symtab_node. No big deal, I fixed that breakage as well. (I'm not sure I have them all actually. symtab_node fields and varpool_node fields are not clearly demarcated into those private to cgraph and those that matter externally.) I could have stopped there too. The final piece was wondering why Jan had added an extra test on DECL_SECTION, seeing that gcc has not reported an error on a changed section attribute for a very long time, and noticing that tls model wasn't generally merged. This is obviously where I went wrong. Silly me. I should have just pointed out these problems and asked you as C front end maintainer to please think about fixing them. That way you would have written the patch, and presumably the testsuite, and everyone would be happy! In fact, maybe I should have just avoided all this effort and just asked Jan to please fix his bug. BTW, in my patch submission at https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01146.html I said "trunk does not honour the last section attribute". What I really should have said was "trunk, with a straight-forward fix for complete loss of section attribute, does not honour the last section attribute". -- Alan Modra Australia Development Lab, IBM
