I'm not trying to pre-empt any feedback from John McCall just ensure the patch doesn't get lost. However if you're happy to commit it now then that's sounds good to me. I don't have commit rights, so if you could do that for me please.
-----Original Message----- From: David Blaikie [mailto:[email protected]] Sent: 10 September 2012 19:54 To: David Tweed Cc: John McCall; [email protected]; 陳韋任 Subject: Re: [cfe-commits] [PATCH v2] Fixing some clang tests where ARM C++ ABI differs On Mon, Sep 10, 2012 at 11:41 AM, David Tweed <[email protected]> wrote: > ping on patch review status: the original submission did have a couple of > issues pointed out in review, primarily because it wasn't quite the patch I > intended to send. With the explanation of the alternating choice of triples > and the correct patch sent, is this suitable for committing in order to fix > those test failures on ARM? I had half a mind to wait for John to OK the alternating choices of triples, but honestly that minor detail seems fine for post-commit feedback if he feels strongly about it. LGTM, please commit - or I can do that for you if you don't have commit rights. - David > > -----Original Message----- > From: David Tweed [mailto:[email protected]] > Sent: 07 September 2012 08:56 > To: 'David Blaikie' > Cc: John McCall; [email protected]; 陳韋任 > Subject: RE: [cfe-commits] [PATCH v2] Fixing some clang tests where ARM C++ > ABI differs > > Hi, thanks for looking at this. I'm basically moved to a new work > environment and I'm still ironing out some of the systems issues. Firstly, > I'm working on the bureaucracy getting a better mail agent installed; please > bear with me. > > On Thu, Sep 6, 2012 at 10:17 AM, David Tweed <[email protected]> wrote: >> Hi, >> >> I've reworked the patch taking into account the comments. Could someone >> review it with a view towards committing please: >> >> There are several clang C++ tests which are failing when run on ARM purely >> due to a combination of (1) being run without an explicit triple and (2) > the >> ARM C++ ABI specifying constructors/destructors return the "this" pointer >> (whereas itanium based C++ ABIs return void). This patch converts these >> tests so that each one is run with a specific triple forcing a > well-defined >> constructor/destructor signature. Most of the tests use the x86-64 triple, >> but a couple use the > > | Incomplete sentence? > > Yep, patch description should say > > "Most of the tests use the x86-64 triple, but a couple use the > armv7-none-eabi." > > | I'm curious why you chose the armv7-none-eabi triple on some of the > | cases? (not that variety is necessarily bad, but lack of consistency > | just makes me wonder whether it's deliberate (& if so, what the reason > | is) or incidental) > > Bear with me as I argue for motherhood and apple pie... In an ideal world we > want fully working code which causes the reghression tests not to encounter > any problems, rather than just to do something to get the regression tests > to pass. So it would be helpful in stopping regressions on the ARM port to > be testing some stuff on there, just as x86 gets very extensive test > coverage. (Though I'm open to the argument that given the manpower levels > only having test writers need to be aware of one ABI might be the best > pragmatic solution.) One of the reasons I'm looking at this is a desire to > have ARM being tested effectively in the tree. > > Regarding the choice, I just went through the files in the original diff in > order of appearance alternating between x86-64 and ARM to get roughly 50 per > cent coverage each. > > | Also on the first (CodeGenCXX/copy-constructor-elim-2.cpp) and last > | (test/CodeGenCXX/throw-expression-cleanup.cpp) tests you added RUN > | lines rather than modifying the existing RUN lines. Won't that mean > | you'll still get failures from the original RUN lines in those tests? > > Screw up on my part: I got the tests running on ARM, copied the diff to x86 > to fix anything I'd broken there but then attached the wrong diff file. > Attached is a the patch fixing that oversight, and running git-diff without > a prefix. (I'm also working on simplifying the multi-platform workflow so > hopefully I won't make this mistake again.) > > Regards, > David > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
