ymand wrote: This review has been going on for longer than a month so, as one of the reviewers and the original author of this check, I wanted to jump in and see if we can bring it to an agreeable conclusion. I’d like to start with some background:
* This check was introduced in 2020 with the current limitations of poor understanding of tests and was raised as an [issue in May 2023](https://github.com/llvm/llvm-project/issues/62600). Our codebase does not distinguish test folders for the most part, so we relied on regexp matching of file names, which never really worked (the patterns match both too much and too little). * This check is built on the Clang Dataflow Framework, which takes primary responsibility for modeling code. The check simply wraps the `optional` model from that framework. In that project (of which I am a lead and active contributor), we have not prioritized modeling test code, because modeling the large variety of test macros correctly is quite difficult and we see it as a lower value than improving the framework and models for normal code. We would love to have better coverage, but given our limited resources, this is our prioritization. * Despite this limitation being present in this checker since 2020, no one in the broader community has ever expressed an interest in picking up this work. We take that as a (weak) signal of lack of user prioritization for this feature as well. For codebases with dedicated test directories, that makes a lot of sense. * We (jvoung and I) cannot commit to the level of work required to fully support the variety of googletest matchers — not least ASSERT_THAT, for which we've seen significant use with optional values. We certainly can’t commit to support beyond googletest, which is a real issue, as [5chmidti](https://github.com/5chmidti)'s comment about `catch2` and `REQUIRE` highlights. With all this, we view it as necessary, for codebases without dedicated test directories, to offer this feature in the checker. Not as a short-term fix, but as a long term feature. As we progress (hopefully) in coverage of test macros, the users of this feature will be able to gradually switch off of it — starting with those users that only rely on the relatively simple ASSERT_TRUE/FALSE and continuing from there. Given how long this issue has remained open, I hope we can resolve and reach a conclusion soon. Thanks! Yitzhak https://github.com/llvm/llvm-project/pull/115051 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
