On Wed, Aug 28, 2013 at 9:55 AM, David Majnemer <[email protected]> wrote: > > > > On Wed, Aug 28, 2013 at 8:04 AM, David Blaikie <[email protected]> wrote: >> >> On Wed, Aug 28, 2013 at 2:06 AM, David Majnemer >> <[email protected]> wrote: >> > On Mon, Aug 26, 2013 at 1:09 PM, Reid Kleckner <[email protected]> wrote: >> >> >> >> On Sun, Aug 25, 2013 at 8:03 PM, David Blaikie <[email protected]> >> >> wrote: >> >>> >> >>> On Sun, Aug 25, 2013 at 8:02 PM, David Majnemer >> >>> <[email protected]> wrote: >> >>> > On Sun, Aug 25, 2013 at 7:59 PM, David Blaikie <[email protected]> >> >>> > wrote: >> >>> >> >> >>> >> On Sun, Aug 25, 2013 at 7:35 PM, David Majnemer >> >>> >> <[email protected]> wrote: >> >>> >> > Author: majnemer >> >>> >> > Date: Sun Aug 25 21:35:51 2013 >> >>> >> > New Revision: 189208 >> >>> >> > >> >>> >> > URL: http://llvm.org/viewvc/llvm-project?rev=189208&view=rev >> >>> >> > Log: >> >>> >> > [-cxx-abi microsoft] Unnamed types are mangled less wrong >> >>> >> >> >>> >> Test case? >> >>> > >> >>> > >> >>> > <unnamed-tag> is still wrong, <unnamed-tag>@ is just marginally less >> >>> > wrong. >> >>> > I thought of this change more of a code cleanup than a bug-fix for >> >>> > mangling. >> >>> >> >>> A change in behavior really ought to have a test. If it's still wrong, >> >>> a FIXME showing the ring mangling in the test case should suffice. >> >>> >> >>> The fact that this didn't break any existing tests seems to indicate >> >>> that this area lacks coverage - adding tests now, even if they >> >>> demonstrate the broken behavior & document what it should be, might be >> >>> nice, so we can track progress towards correctness. >> >> >> >> >> >> Right, I'd CHECK for the current mangling and have a FIXME with the >> >> desired mangling. >> > >> > >> > We already have a PR tracking the broken behavior. I am not aware of any >> > LLVM policy, codified or implicit, that asks for bug PRs to be encoded >> > in >> > the test suite. >> >> There isn't, though it's been discussed as a possible idea (one that I >> rather like). >> >> The relevant policy here is: intentional behavioural changes have >> tests for the changed behaviour. > > > And I fully agree with that policy, I just didn't see it as a behavioral > change. In my eyes, that code was still there purely for illustrative > reasons so that I remember to '@' terminate when I came back to fix it for > real.
If all you wanted was a reminder, changing the production code semi-arbitrarily (in the sense that it's untested and/or dead/broken/etc (still) after your change) is perhaps unnecessary. A test case testing the failing behavior with a FIXME showing the desired behavior would be a good reminder for you or anyone else who implements this in the future. (or, if you're going hardcore, a test case testing the correct behavior, set to XFAIL - but given the granularity of our XFAILs (whole test file) it's a bit unfortunate to haev to have a separate test file for the issue, or lose coverage on other things tested in the same file). Such tests are not uncommon, though moreso in "fuzzy" areas like diagnostics "so I fixed this diagnostic issue, but the code still produces bad diagnostics for this corner case". I suppose an underlying issue here is that the codebase should be self documenting - being in a state where there's code in the codebase that's untested & unjustified is bad. We shouldn't knowingly create or perpetrate this state. Yes, there are cases where this is already true, but that's a legacy we have to live with. Not sure how else I can put this but changes that have observable effects really should include tests. - David _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
