[Patch][ObjC][Proposal] NSValue literals

[John McCall]

(Please disregard the earlier version of this message if you received it.
Also, I’ve removed Fariborz from the CC list, as he recently retired.)

> On May 28, 2015, at 8:29 AM, AlexDenisov <1101.debian at gmail.com> wrote:
> 
> Hi there.
> Here is the final (from my point of view) version.
> I can continue working on fixes/improvements after the review.
> Current version includes:
> 
> - new ‘objc_boxed_nsvalue_expression’ feature
> - new ‘objc_boxable’ attribute
> - ObjCBoxedExpr with limited amount of sub-expressions
> - Sema/CodeGen tests
> - support of lvalues and rvalues
> - ‘c-index-test’ and ‘ASTReader/ASTWriter’ adopted to support new ObjCBoxedExpr format, added tests to check if they’re working properly
> - ‘ObjectiveCLiterals.rst’ includes all new features (though wording needs to be improved)
> 
> I guess I’m missing something here, but it’s just a brief overview of the content of the patch.
> 
> Looking forward for your feedback.

I like the basic approach, but I have some requests before it’s ready.

--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -48,9 +48,10 @@ Major New Features
------------------

-  requires passing the -fms-extensions or -fborland compiler flag. This language
-  extension is also enabled when compiling CUDA code, but its use should be
-  viewed as an implementation detail that is subject to change.
+requires passing the -fms-extensions or -fborland compiler flag. This language
+extension is also enabled when compiling CUDA code, but its use should be
+viewed as an implementation detail that is subject to change.
+

I don’t think you meant to include this change.

+
+  /// ObjCBoxable - This is true if this struct can be boxed into NSValue
+  bool ObjCBoxable : 1;
+

You haven’t changed serialization to record this bit, but fortunately there’s
no need to; just remove it.  It’s completely acceptable to just test for the
attribute with hasAttr<ObjCBoxableAttr>().

class ObjCBoxedExpr : public Expr {
-  Stmt *SubExpr;
+  // Most expressions need only one sub-expression (e.g. @("hello world")),
+  // but NSValue expressions need two parameters:
+  // actual expression (e.g. view.frame) and @encode (e.g. @encode(CGRect))
+  SmallVector<Stmt *, 2> SubExprs;
  ObjCMethodDecl *BoxingMethod;
  SourceRange Range;
+  unsigned NumSubExprs;

SmallVectors aren’t allowed in the AST; they leak memory because we don’t run
destructors on AST nodes.  Fortunately, it’s not necessary; it’s straightforward to
change IRGen to directly generate the @encode value when the boxing method
takes two arguments.

You’ll want to change GetAddrOfConstantStringFromObjCEncode to take the
type instead of the ObjCEncodeExpr.

+bool Type::isObjCBoxableStructureType() const {
+  if (const RecordType *RT = getAs<RecordType>())
+    return RT->getDecl()->isStruct() && RT->getDecl()->isObjCBoxable();
+  return false;
+}

Testing for struct is unnecessary; a union is theoretically fine.  Even if you want
to impose this restriction, you should impose it as a formation restriction, i.e. you
should diagnose applying the attribute to the declaration instead of silently
ignoring it in specific cases later.

+static void handleObjCBoxable(Sema &S, Decl *D, const AttributeList &Attr) {
+  RecordDecl *RD = nullptr;
+  if (TypedefDecl *TD = dyn_cast<TypedefDecl>(D)) {
+    const RecordType *RT = TD->getUnderlyingType()->getAs<RecordType>();
+    RD = RT->getDecl();

This potentially changes earlier declarations, which requires more logic to
handle correctly — you'll need to create an AST update record if you want
to do this.

I recommend just handling the case where the target declaration is a RecordDecl.

  ValueExpr = RValue.get();
  QualType ValueType(ValueExpr->getType());
-  if (const PointerType *PT = ValueType->getAs<PointerType>()) {
-    QualType PointeeType = PT->getPointeeType();
-    if (Context.hasSameUnqualifiedType(PointeeType, Context.CharTy)) {
+  if (ValueType->isBuiltinType() || ValueType->isEnumeralType()) {
+    // We support numeric, char, BOOL/bool and Enum types.
+    // Enum types treat as Integer.

I’m not sure why you needed to restructure this code instead of just adding
another case testing for RecordType after the EnumType case.

+  } else if (ValueType->isObjCBoxableStructureType()) {
+    // Support for structure types, that marked as objc_boxable
+    // struct s { ... } __attribute__((objc_boxable));

The attribute has to be written after the “struct” keyword to apply to the struct.

As mentioned above, you don’t need to construct the @encode string
expression here.  It’s a nicer representation to just store the original
sub-expression.

There are some extra things you need to do here to do the right thing in
ObjC++.  (In principle, it would be better to check them when applying the
attribute, but we can’t rely on the declaration being complete at that point.)

The first is that DefaultFunctionArrayLvalueConversion does not actually
convert l-values of struct/union type to r-values in C++.  You’ll need to do this:

 if (getLangOpts().CPlusPlus && ValueExpr->isGLValue()) {
   ExprResult Temp = PerformCopyInitialization(
                      InitializedEntity::InitializeTemporary(ValueExpr->getType()),
                                               ValueExpr->getExprLoc(), ValueExpr);
   if (Temp.isInvalid())
     return ExprError();
   ValueExpr = Temp.get();
 }

That will also implicitly check that the type is complete.

The second is that you should test whether the type is trivially copyable
(with ValueType->isTriviallyCopyableType(Context)) and diagnose if it isn’t.

John.