> On 2015-Feb-20, at 14:17, Steven Wu <[email protected]> wrote: > > Hi dexonsmith, gottesmm, > > Currently, there is no good way to tell if the bitcode is compiled > with ARC or not. However, it is required to run objc-arc-contract > pass for ObjC ARC. Introduces a new module flag for ObjC-ARC and > use that to decide if objc-arc-contract pass is required or not. >
This makes sense to me. You're missing testcases: - Test that the flag gets added correctly (only when it should). - Test that the pass gets run based on the flag. Some other comments inline. > http://reviews.llvm.org/D7799 > > Files: > lib/CodeGen/BackendUtil.cpp > lib/CodeGen/CGObjCMac.cpp > > Index: lib/CodeGen/BackendUtil.cpp > =================================================================== > --- lib/CodeGen/BackendUtil.cpp > +++ lib/CodeGen/BackendUtil.cpp > @@ -568,7 +568,7 @@ > // Add ObjC ARC final-cleanup optimizations. This is done as part of the > // "codegen" passes so that it isn't run multiple times when there is > // inlining happening. > - if (LangOpts.ObjCAutoRefCount && > + if (TheModule->getModuleFlag("Objective-C ARC") && > CodeGenOpts.OptimizationLevel > 0) > PM->add(createObjCARCContractPass()); > > Index: lib/CodeGen/CGObjCMac.cpp > =================================================================== > --- lib/CodeGen/CGObjCMac.cpp > +++ lib/CodeGen/CGObjCMac.cpp > @@ -4283,6 +4283,13 @@ > } > } > > + // Add ObjCAutoRefCount as a module flag I don't think this comment is helpful. It's clear from the code what's happening. Be sure the commit message explains *why* the logic is this way, though. > + if (CGM.getLangOpts().ObjCAutoRefCount) { > + // AutoRefCount will overwrites files which don't have ARC. I don't think this comment is helpful. > + Mod.addModuleFlag(llvm::Module::Override, > + "Objective-C ARC", (uint32_t)1); I think `1u` is easier to read than `(uint32_t)1` here. Also, I think `Error` should be sufficient. There's nothing to override here (we never set it to anything but `1`). > + } > + > // Indicate whether we're compiling this to run on a simulator. > const llvm::Triple &Triple = CGM.getTarget().getTriple(); > if (Triple.isiOS() && _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
