[libcxx-commits] [PATCH] D115795: [libc++] Add GCC workaround in std::char_traits<char>::length()

Thu Dec 16 06:29:32 PST 2021

ldionne added a comment.

In D115795#3196141 <https://reviews.llvm.org/D115795#3196141>, @Quuxplusone wrote:

> In D115795#3195991 <https://reviews.llvm.org/D115795#3195991>, @ldionne wrote:
>
>> In D115795#3195968 <https://reviews.llvm.org/D115795#3195968>, @Quuxplusone wrote:
>>
>>> With one touch of libcxx/test/, ta-da, buildkite detected the typo for you.
>>
>> I'm not sure I understand this -- are you implying that BuildKite doesn't trigger tests if `libcxx/test` isn't touched?
>
> Not at all. "If it's not being tested, then it doesn't work" is an early-2000s-Green-Hills-Software mantra, but it's more or less the same idea as "If you liked it then you should have put a test on it." But a little less emphasis on interpersonal motivations and more just Murphy's Law: If some feature //could// be broken without making the build red, then I can pretty much guarantee you that it //is// broken right now.
>
> Thus, whenever there's a code change, there had better be a test change too: Either we're fixing a previously-untested codepath (as in this case) so we should add a test, or we're fixing a previously-wrong codepath so we should modify a test. (Or it's completely NFC, which also wasn't the case here.) In the original submission, there was a code change but zero test changes; I said "Code LGTM but I'd //expect// to see a test change too" (because of everything I just said). And then @philnik added that test change, and lo and behold, that change //immediately// identified a typo-bug that had slipped past (I'm assuming) both of us!
> If (hypothetically, and maybe you think this'd never happen ;)) we had all LGTM'ed the code and shipped it, it would have totally broken constexpr string_view on GCC. "Constexpr string_view on GCC" is the thing in this instance that we weren't testing (because it was XFAIL'ed), and therefore didn't work... and therefore, even after we thought "yeah, this should fix it," it actually //continued// not-to-work... until we put a test on it. :)
>
> None of this has any particular ramifications for this PR at all. I'm just pointing at it and saying "Look, perfect minimal gem of an illustration as to why code changes should imply test changes!" This was like an 8-line diff, "obviously" correct. Staring at the code didn't find the typo (at least, not in my case). Mindless (at least, in my case ;)) application of the mantra led to its being found.

What I had misunderstood is that the test kept failing due to a typo and was hence "passing" the XFAIL test, but for a different reason. I understand and agree with everything you've said now that I understand that. Thanks for clarifying though.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115795/new/

https://reviews.llvm.org/D115795