[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty

[Josh Learn via Phabricator via cfe-commits]

guitard0g marked 6 inline comments as done.
guitard0g added inline comments.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:6683
+  values.add(classMethodList);
+  isEmptyCategory &=
+      instanceMethodList->isNullValue() && classMethodList->isNullValue();
----------------
zoecarver wrote:
> Does this really need to be an `&=`? Is there any case where `isEmptyCategory` isn't `true` here?
Good point, fixed. 


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:6713
   }
 
+  unsigned Size =
----------------
ahatanak wrote:
> Is it not possible to check whether the category is empty here? If it's empty, you can avoid creating the global variable and abandon the builder.
Good point, done. 


================
Comment at: clang/test/CodeGenObjC/category-class-empty.m:1
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O3 -emit-llvm -o - %s | FileCheck %s
+// PR7431
----------------
jkorous wrote:
> zoecarver wrote:
> > Is `-O3` needed here?
> I would even think `-O0` is better and consider also `-disable-llvm-passes`. Ultimately the goal is to make sure frontend codegen doesn't emit the metadata so the less processing of the IR in the backend the more sensitive the test will be.
Good catch, changed to -O0 with -disable-llvm-passes. 


================
Comment at: clang/test/CodeGenObjC/category-class-empty.m:10
+ at interface A(foo)
+- (void)foo_myStuff;
+ at end
----------------
jkorous wrote:
> I assume all the cases when we want to emit the metadata (at least one instance method, at least one class method, ...) are covered by other tests, right?
> If there's a case that's missing we should add a test for it.
Yeah a fair number of tests would break if we started removing categories with anything inside of them. 


================
Comment at: clang/test/CodeGenObjC/non-lazy-classes.m:45
 __attribute__((objc_nonlazy_class))
- at implementation E (MyCat) @end
+ at implementation E (MyCat)
+-(void) load {
----------------
zoecarver wrote:
> This is to prevent another test from failing? Makes sense. 
Yeah this is the one case where the IRGen check was looking for the category even though it's empty. Should be a harmless change. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113455/new/

https://reviews.llvm.org/D113455