[clang] [ObjC] Check entire chain of superclasses to see if class layout can be statically known (PR #81335)
John McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 19 15:16:55 PST 2024
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
More information about the cfe-commits
mailing list