https://github.com/AaronBallman commented:
Yeah, these changes really need to come with test coverage. We allow landing changes with no test coverage when it's an NFC change or when the testing would require heroic efforts. Otherwise, we expect tests that are as good as you can reasonably make them (sometimes that means going with a unit test instead of a lit test, sometimes it requires creativity in lit, etc). You don't have to fix the testing system first, it's okay to land tests in an unideal state with a FIXME comment, for example. In terms of the code changes, they look mostly reasonable (some minor nits for coding style conformance) and should be something we can test. We used to crash, so the test is demonstrating that we no longer crash (and that means verifying some particular set of diagnostics are now generated instead or that some IR is now generated, etc). Such a test would be good to have a comment explaining that the code used to crash with a mention or link to the github issue(s). https://github.com/llvm/llvm-project/pull/78898 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
