r189208 - [-cxx-abi microsoft] Unnamed types are mangled less wrong

[David Blaikie]

On Wed, Aug 28, 2013 at 9:55 AM, David Majnemer
<david.majnemer at gmail.com> wrote:
>
>
>
> On Wed, Aug 28, 2013 at 8:04 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Wed, Aug 28, 2013 at 2:06 AM, David Majnemer
>> <david.majnemer at gmail.com> wrote:
>> > On Mon, Aug 26, 2013 at 1:09 PM, Reid Kleckner <rnk at google.com> wrote:
>> >>
>> >> On Sun, Aug 25, 2013 at 8:03 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >>>
>> >>> On Sun, Aug 25, 2013 at 8:02 PM, David Majnemer
>> >>> <david.majnemer at gmail.com> wrote:
>> >>> > On Sun, Aug 25, 2013 at 7:59 PM, David Blaikie <dblaikie at gmail.com>
>> >>> > wrote:
>> >>> >>
>> >>> >> On Sun, Aug 25, 2013 at 7:35 PM, David Majnemer
>> >>> >> <david.majnemer at gmail.com> 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