[Patch][ObjC][Proposal] NSValue literals

[John McCall]

> 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.

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.

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

There’s no need to store this directly on the RecordDecl, and doing so greatly
complicates what you need to do.  Just check for an 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.  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.

You can test for the attribute with hasAttr<ObjCBoxableAttr>().  You might
have to do this in a .cpp file.

+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.

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.