[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