[PATCH] D53547: NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects)

Jim Ingham via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 23 11:17:39 PDT 2018



> On Oct 23, 2018, at 11:09 AM, Erik Pilkington via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> erik.pilkington added a subscriber: jingham.
> erik.pilkington added inline comments.
> 
> 
> ================
> Comment at: clang/lib/Basic/IdentifierTable.cpp:166-167
>   // in non-arc mode.
> -  if (LangOpts.ObjC2 && (Flags & KEYARC)) return KS_Enabled;
> -  if (LangOpts.ObjC2 && (Flags & KEYOBJC2)) return KS_Enabled;
> +  if (LangOpts.ObjC && (Flags & KEYARC)) return KS_Enabled;
> +  if (LangOpts.ObjC && (Flags & KEYOBJC)) return KS_Enabled;
>   if (LangOpts.ConceptsTS && (Flags & KEYCONCEPTS)) return KS_Enabled;
> ----------------
> rsmith wrote:
>> Would it make sense to fold `KEYOBJC` and `KEYARC` together?
> Yep, good point. Looks like we used to only enable these keywords in -fobjc-arc mode, but now that we're doing it for objective-c there isn't any distinction to be made here. I'll commit this in a follow-up.
> 
> 
> ================
> Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:408
>     // non-Apple platforms, but for now it is needed.
> -    m_compiler->getLangOpts().ObjC1 = true;
> +    m_compiler->getLangOpts().ObjC = true;
>     break;
> ----------------
> rsmith wrote:
>> Curious, this looks like it was the *only* way we previously ever got into `ObjC1` mode. Any idea why this path turns on `ObjC1` but not `ObjC2`?
> This comes from https://reviews.llvm.org/D11102. I can't really imagine this being anything but an oversight, I doubt that there is some subtle reason to use ObjC1. @jingham: Can you confirm that there isn't any reason to use just ObjC1 and not ObjC1+ObjC2 here?

This certainly does seem like an oversight, and I can't think of any good reason why this change just set ObjC1 and not ObjC2 like all the other places this was done.

Jim


> 
> 
> https://reviews.llvm.org/D53547
> 
> 
> 



More information about the cfe-commits mailing list