[PATCH] D152689: [NFC] Remove dead conditionals

Mon Jun 12 13:05:58 PDT 2023

aaron.ballman accepted this revision.
aaron.ballman added inline comments.

================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:3812
   llvm::GlobalVariable *GV;
-  if (ForClass)
-    GV =
----------------
rjmccall wrote:
> aaron.ballman wrote:
> > I agree this is dead code (see line 3777-3778 above), but @rjmccall is it a bug that we bail out with a null value (e.g, is the correct fix to remove the above lines)?
> No, I don't think this is a bug, and that would not be the right fix anyway.  `ForClass` means we're building the metaclass, and subclasses do not introduce new ivars in the metaclass.  The comment is saying that GCC declares ivars for the class structure when emitting metaclasses for root classes, but if we haven't been doing that for 15 years, I don't see a reason to start.  Certainly the runtime doesn't require it in order to work.  So the patch as written looks good.
> 
> For what it's worth, this code is all but dead for more basic reasons. `CGObjCMac` is the legacy ObjC ABI, which is no longer used on any platform that's even vaguely supported by Apple.  The last platform we shipped using this ABI was i386 macOS; we haven't sold Macs that only supported i386 since 2006, and we dropped the i386 userspace entirely in macOS Catalina (2019).  So the only reason for this code to exist is to allow people to either compile programs for very old hardware or intentionally use a 32-bit ABI on newer hardware running an old OS.
Should we consider removing this code at this point? Folks who need to compile programs for very old hardware or have esoteric ABI needs can stick with an older release of Clang (not ideal, but I don't think it makes much sense to continue to carry around unsupported targets).

If so, we could add a release note/diagnostic for Clang 17 and then rip the code out in Clang 18 unless users have compelling use cases.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152689