[PATCH] D20407: [CodeGen][ObjC] zero-ext an i1 value to i8

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: include/clang/AST/Type.h:1084
@@ +1083,3 @@
+  /// Strip typedefs and atomic from the given type.
+  QualType getDesugaredAtomicValueType(const ASTContext &ctx) const;
+
----------------
Please name this getAtomicUnqualifiedType() and have it promise to remove all qualifiers including _Atomic.  It should not promise to remove type sugar.

================
Comment at: lib/AST/Type.cpp:1282
@@ +1281,3 @@
+  return *this;
+}
+
----------------
This should just be:
  if (auto AT = getAs<AtomicType>()) {
    return AT->getValueType().getUnqualifiedType();
  } else {
    return getUnqualifiedType();
  }

================
Comment at: lib/CodeGen/CGObjC.cpp:901
@@ -903,1 +900,3 @@
+    uint64_t IVarSize = getContext().toBits(strategy.getIvarSize());
+    llvm::Type *bitcastType = llvm::Type::getIntNTy(getLLVMContext(), IVarSize);
     bitcastType = bitcastType->getPointerTo(); // addrspace 0 okay
----------------
The code around here uses lowercase identifiers, please be consistent.

================
Comment at: lib/CodeGen/CGObjC.cpp:1023
@@ -1014,1 +1022,3 @@
+            value,
+            ConvertType(propType.getDesugaredAtomicValueType(getContext())));
         value = Builder.CreateBitCast(
----------------
I don't understand the purpose of doing two bitcasts here, so let's just drop this first one.

================
Comment at: lib/Sema/SemaDeclObjC.cpp:4319
@@ +4318,3 @@
+      Context, MethodLoc, EndLoc, Sel,
+      resultDeclType.getDesugaredAtomicValueType(Context),
+      ReturnTInfo, CurContext, MethodType == tok::minus, isVariadic,
----------------
Hmm.  I'm not sure you should be doing this here; or at least, if we're going to do it, we should probably be doing it consistently in CheckFunctionReturnType.

================
Comment at: lib/Sema/SemaDeclObjC.cpp:4361
@@ -4361,1 +4360,3 @@
+        ObjCMethod, StartLoc, ArgInfo[i].NameLoc, ArgInfo[i].Name,
+        ArgType.getDesugaredAtomicValueType(Context), DI, SC_None);
 
----------------
Same thing.  If the user wants to explicitly declare a parameter _Atomic, they can do so.

================
Comment at: lib/Sema/SemaObjCProperty.cpp:1497
@@ -1496,2 +1496,3 @@
   QualType PropertyIvarType = property->getType().getNonReferenceType();
+  PropertyIvarType = PropertyIvarType.getDesugaredAtomicValueType(Context);
   bool compat = Context.hasSameType(PropertyIvarType, GetterType);
----------------
This is incorrect.  The property ivar type should definitely remain _Atomic if the property was declared that way.

================
Comment at: lib/Sema/SemaObjCProperty.cpp:2210
@@ -2209,1 +2209,3 @@
+    QualType resultTy =
+        property->getType().getDesugaredAtomicValueType(Context);
     if (property->getPropertyAttributes() &
----------------
Please pull this above the comment and give it its own comment, something like:
  // The getter returns the declared property type with all qualifiers removed.

================
Comment at: lib/Sema/SemaObjCProperty.cpp:2282
@@ -2279,2 +2281,3 @@
       QualType paramTy = property->getType().getUnqualifiedType();
+      paramTy = paramTy.getDesugaredAtomicValueType(Context);
       if (property->getPropertyAttributes() &
----------------
Same thing: please pull this above and give it its own comment.


http://reviews.llvm.org/D20407