https://github.com/rjmccall commented:
Thanks, the breadth of tests looks good. Please improve the actual testing, though — CHECK lines are testing the IR output file at a whole, so now that there are multiple tests in this file, we really need to make these tests more specific. - First, please add CHECK-LABEL lines for the functions you're trying to test so that we know that the checks are actually applying to the code in that function. - Second, I know the existing test is doing this CHECK-NOT thing, but it should really always have been testing that the ivar access actually uses the right offset; please do that, both for your new tests and the existing one. - Finally, please make sure you test at least one access for each of the ivars. Putting accesses in separate methods will help with ensuring that you're testing each specifically. As a procedural note, I was going to review this last week after you responded to the review, but then I saw that you kept uploading new versions of the patch, so I held off. I certainly understand needing to update the patch a few times; I do that myself all the time. Just make sure you give me a heads up that the patch has been updated and is actually ready for review: either hold your response until your patch is ready, or make a short comment like "Sorry about that, patch is ready for review now." when the updates are done. https://github.com/llvm/llvm-project/pull/81335 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
