Struct constructors work like built-in type constructors. (#751)

You know how you can have an expression for a built-in type like color?

    foo = color(1,1,1)

or

    void func (color c) { ...}
    func (color(1,1,1);

It was really awkward to do this with structs.

So with this patch, for every struct, you can construct it by name
with a function-call-like syntax, where the arguments must be the same
types as the data members of the struct, in order.

For example:

    struct RGBA {
        color rgb;
        float a;
    };

    RGBA x = RGBA(color(1,1,1), 1);

    void func (RGBA x) { ... }
    func (RGBA(Cblah,alpha));

You don't need to explicitly declare the constructor like in C++, it
just exists.

Author: lgritz

CMakeLists.txt

@@ -257,7 +257,8 @@ TESTSUITE ( and-or-not-synonyms aastep arithmetic array array-derivs array-range
             oslc-err-format oslc-err-intoverflow
             oslc-err-noreturn oslc-err-notfunc
             oslc-err-outputparamvararray oslc-err-paramdefault
-            oslc-err-struct-array-init oslc-err-struct-dup
+            oslc-err-struct-array-init oslc-err-struct-ctr
+            oslc-err-struct-dup
             oslc-warn-commainit
             oslc-variadic-macro
             oslc-version

src/doc/languagespec.tex

@@ -62,7 +62,7 @@
 \emph{lg@imageworks.com}
 }
 \date{{\large Date: 6 Feb 2017 \\
- (with corrections, 28 Apr 2017)
+ (with corrections, 12 May 2017)
 }
 \bigskip
 \bigskip
@@ -2068,14 +2068,37 @@ \section{Structures}
 is similar to C or C++:
 
 \begin{code}
-    struct Ray {                   // Define a structure type
-        point pos;
-        vector dir;
+    struct RGBA {                    // Define a structure type
+        color rgb;
+        float alpha;
     };
 
-    Ray r;                         // Declare a structure
-    r.pos = point (1, 0, 0);       // Assign to one field
-    point p = r.pos;               // Read from a structure field
+    RGBA col;                        // Declare a structure
+    r.rgb = color (1, 0, 0);         // Assign to one field
+    color c = r.rgb;                 // Read from a structure field
+
+    RGBA b = { color(.1,.2,.3), 1 }; // Member-by-member initialization
+\end{code}
+
+You can use ``constructor expressions'' for a your struct types much like
+you can construct built-in types like \color or \point:
+
+\vspace{10pt}
+\hspace{0.25in} \emph{struct\_name ( first\_member\_value, ... )}
+\vspace{10pt}
+
+\noindent For example,
+
+\begin{code}
+    RGBA c = RGBA(col,alpha);        // Constructor syntax
+
+    RGBA add (RGBA a, RGBA b)
+    {
+        return RGBA (a.rgb+b.rgb, a.a+b.a);   // return expression
+    }
+
+    // pass constructor expression as a parameter:
+    RGBA d = add (c, RGBA(color(.3,.4,.5), 0));
 \end{code}
 
 It is permitted to have a structure field that is an array, as well as to
@@ -2089,8 +2112,8 @@ \section{Structures}
         float b[4];       // Ok: struct may contain an array
     };
 
-    Ray rays[4];          // Ok: Array of structures
-    rays[0].pos = 0;
+    RGBA vals[4];         // Ok: Array of structures
+    vals[0].a = 0;
 
     A d[5];               // NO: Array of structures that contain arrays
 \end{code}

src/liboslcomp/ast.cpp

@@ -998,6 +998,9 @@ ASTfunction_call::ASTfunction_call (OSLCompilerImpl *comp, ustring name,
         // find the things that almost matched and offer suggestions.
         return;
     }
+    if (is_struct_ctr()) {
+        return;  // It's a struct constructor
+    }
     if (m_sym->symtype() != SymTypeFunction) {
         error ("'%s' is not a function", name.c_str());
         m_sym = NULL;

src/liboslcomp/ast.h

@@ -339,6 +339,33 @@ class ASTNode : public OIIO::RefCnt {
                                 bool copywholearrays, int intindex,
                                 bool paraminit);
 
+    // Helper: type check an initializer list -- either a single item to
+    // a scalar, or a list to an array.
+    void typecheck_initlist (ref init, TypeSpec type, string_view name);
+
+    // Helper: generate code for an initializer list -- either a single
+    // item to a scalar, or a list to an array.
+    void codegen_initlist (ref init, TypeSpec type, Symbol *sym);
+
+    // Special type checking for structure member initializers.
+    // It's in the ASTNode base class because it's used from mutiple
+    // subclasses.
+    TypeSpec typecheck_struct_initializers (ref init, TypeSpec type,
+                                            string_view name);
+
+    // Special code generation for structure initializers.
+    // It's in the ASTNode base class because it's used from mutiple
+    // subclasses.
+    Symbol *codegen_struct_initializers (ref init, Symbol *sym,
+                                         bool is_constructor=false);
+
+    // Helper for param_default_literals: generate the string that gives
+    // the initialization of the literal value (and/or the default, if
+    // init==NULL) and append it to 'out'.  Return whether the full
+    // initialization is comprised only of literals (no init ops needed).
+    bool one_default_literal (const Symbol *sym, ASTNode *init,
+                      std::string &out, string_view separator=" ") const;
+
     void error_impl (string_view msg) const;
     void warning_impl (string_view msg) const;
 
@@ -446,33 +473,11 @@ class ASTvariable_declaration : public ASTNode
 
     void codegen_initializer (ref init, Symbol *sym);
 
-    // Special code generation for structure initializers
-    Symbol *codegen_struct_initializers (ref init, Symbol *sym);
-
     void register_struct_init (ustring name, ASTNode *init) {
         m_struct_field_inits.emplace_back(name, init);
     }
 
 private:
-    // Helper: type check an initializer list -- either a single item to
-    // a scalar, or a list to an array.
-    void typecheck_initlist (ref init, TypeSpec type, const char *name);
-
-    // Special type checking for structure initializers
-    TypeSpec typecheck_struct_initializers (ref init, TypeSpec type,
-                                            const char *name);
-
-    // Helper: generate code for an initializer list -- either a single
-    // item to a scalar, or a list to an array.
-    void codegen_initlist (ref init, TypeSpec type, Symbol *sym);
-
-    // Helper for param_default_literals: generate the string that gives
-    // the initialization of the literal value (and/or the default, if
-    // init==NULL) and append it to 'out'.  Return whether the full
-    // initialization is comprised only of literals (no init ops needed).
-    bool param_one_default_literal (const Symbol *sym, ASTNode *init,
-                      std::string &out, const std::string &separator=" ") const;
-
     ustring m_name;     ///< Name of the symbol (unmangled)
     Symbol *m_sym;      ///< Ptr to the symbol this declares
     bool m_isparam;     ///< Is this a parameter?
@@ -859,10 +864,13 @@ class ASTfunction_call : public ASTNode
     FunctionSymbol *func () const { return (FunctionSymbol *)m_sym; }
     ref args () const { return child (0); }
 
+    /// Is it a struct constructor?
+    bool is_struct_ctr () const { return m_sym && m_sym->is_structure(); }
+
     /// Is it a user-defined function (as opposed to an OSL built-in)?
     ///
     bool is_user_function () const {
-        return user_function() && !user_function()->is_builtin();
+        return !is_struct_ctr() && user_function() && !user_function()->is_builtin();
     }
 
     /// Pointer to the ASTfunction_declaration node that defines the user
@@ -893,6 +901,9 @@ class ASTfunction_call : public ASTNode
     /// return false and call an appropriate error().
     bool typecheck_printf_args (const char *format, ASTNode *arg);
 
+    /// Handle "funcion calls" that are really struct ctrs.
+    TypeSpec typecheck_struct_constructor ();
+
     /// Is the argument number 'arg' read by the op?
     ///
     bool argread (int arg) const;

src/liboslcomp/codegen.cpp

@@ -620,8 +620,8 @@ ASTNode::codegen_assign_struct (StructSpec *structspec,
 
 
 bool
-ASTvariable_declaration::param_one_default_literal (const Symbol *sym,
-               ASTNode *init, std::string &out, const std::string &sep) const
+ASTNode::one_default_literal (const Symbol *sym, ASTNode *init,
+                              std::string &out, string_view sep) const
 {
     // FIXME -- this only works for single values or arrays made of
     // literals.  Needs to be seriously beefed up.
@@ -792,7 +792,7 @@ ASTvariable_declaration::param_default_literals (const Symbol *sym,
     for (int i = 0;  i==0 || init;  ++i, init = init->nextptr()) {
         if (i)
             out += separator;
-        completed &= param_one_default_literal (sym, init, out, separator);
+        completed &= one_default_literal (sym, init, out, separator);
         if (! compound || ! init)
             break;
     }
@@ -831,15 +831,14 @@ ASTvariable_declaration::codegen_initializer (ref init, Symbol *sym)
 
 
 void
-ASTvariable_declaration::codegen_initlist (ref init, TypeSpec type,
-                                           Symbol *sym)
+ASTNode::codegen_initlist (ref init, TypeSpec type, Symbol *sym)
 {
     // If we're doing this initialization for shader params for their
     // init ops, we need to take care to set the codegen method names
     // properly.
     bool paraminit = (m_compiler->codegen_method() != m_compiler->main_method_name() &&
-                      (m_sym->symtype() == SymTypeParam ||
-                       m_sym->symtype() == SymTypeOutputParam));
+                      (sym->symtype() == SymTypeParam ||
+                       sym->symtype() == SymTypeOutputParam));
 
     if (type.is_structure()) {
         // Special case -- structure : Recurse to handle each field
@@ -850,9 +849,13 @@ ASTvariable_declaration::codegen_initlist (ref init, TypeSpec type,
             ustring fieldname = ustring::format ("%s.%s", sym->mangled(),
                                                  field.name);
             Symbol *fieldsym = m_compiler->symtab().find_exact (fieldname);
-            std::string out;
-            if (paraminit && param_default_literals(fieldsym, init.get(), out))
-                continue;  // Skip if we had a static initalizer
+            if (paraminit) {
+                ASSERT (nodetype() == variable_declaration_node);
+                ASTvariable_declaration *v = (ASTvariable_declaration *)this;
+                std::string out;
+                if (v->param_default_literals(fieldsym, init.get(), out))
+                    continue;  // Skip if we had a static initalizer
+            }
             codegen_initlist (init, fieldsym->typespec(), fieldsym);
         }
         return;
@@ -954,17 +957,18 @@ ASTvariable_declaration::codegen_initlist (ref init, TypeSpec type,
 
 
 Symbol *
-ASTvariable_declaration::codegen_struct_initializers (ref init, Symbol *sym)
+ASTNode::codegen_struct_initializers (ref init, Symbol *sym,
+                                      bool is_constructor)
 {
     // If we're doing this initialization for shader params for their
     // init ops, we need to take care to set the codegen method names
     // properly.
     bool paraminit = (m_compiler->codegen_method() != m_compiler->main_method_name() &&
-                      (m_sym->symtype() == SymTypeParam ||
-                       m_sym->symtype() == SymTypeOutputParam));
+                      (sym->symtype() == SymTypeParam ||
+                       sym->symtype() == SymTypeOutputParam));
 
     ASSERT (sym->typespec().is_structure());
-    if (init->nodetype() != compound_initializer_node) {
+    if (init->nodetype() != compound_initializer_node && !is_constructor) {
         // Just one initializer, it's a whole struct of the right type.
         Symbol *initsym = init->codegen (sym);
         if (initsym != sym) {
@@ -977,7 +981,9 @@ ASTvariable_declaration::codegen_struct_initializers (ref init, Symbol *sym)
     }
 
     // General case -- per-field initializers
-    init = ((ASTcompound_initializer *)init.get())->initlist();
+    if (init->nodetype() == compound_initializer_node) {
+        init = ((ASTcompound_initializer *)init.get())->initlist();
+    }
     StructSpec *structspec (sym->typespec().structspec());
     for (int i = 0; init && i < structspec->numfields(); init = init->next(), ++i) {
         // Structure element -- assign to the i-th member field
@@ -995,7 +1001,9 @@ ASTvariable_declaration::codegen_struct_initializers (ref init, Symbol *sym)
             // For parameter initialization, don't really generate ops if it
             // can be statically initialized.
             std::string out;
-            if (param_default_literals (fieldsym, init.get(), out))
+            ASSERT (nodetype() == variable_declaration_node);
+            ASTvariable_declaration *v = (ASTvariable_declaration *)this;
+            if (v->param_default_literals (fieldsym, init.get(), out))
                 continue;
 
             // Delineate and remember the init ops for this field individually
@@ -1639,6 +1647,14 @@ ASTfunction_call::argwrite (int arg) const
 Symbol *
 ASTfunction_call::codegen (Symbol *dest)
 {
+    if (is_struct_ctr()) {
+        // Looks like function call, but is actually struct constructor
+        if (! dest)
+            dest = m_compiler->make_temporary (typespec());
+        codegen_struct_initializers (args(), dest, true /*is_constructor*/);
+        return dest;
+    }
+
     // Set up a return destination if not passed one (or not the right type)
     if (! typespec().is_void()) {
         if (dest == NULL || ! equivalent (dest->typespec(), typespec()))

src/liboslcomp/typecheck.cpp

@@ -139,8 +139,7 @@ ASTvariable_declaration::typecheck (TypeSpec expected)
 
 
 void
-ASTvariable_declaration::typecheck_initlist (ref init, TypeSpec type,
-                                             const char *name)
+ASTNode::typecheck_initlist (ref init, TypeSpec type, string_view name)
 {
     // Loop over a list of initializers (it's just 1 if not an array)...
     for (int i = 0;  init;  init = init->next(), ++i) {
@@ -170,8 +169,8 @@ ASTvariable_declaration::typecheck_initlist (ref init, TypeSpec type,
 
 
 TypeSpec
-ASTvariable_declaration::typecheck_struct_initializers (ref init, TypeSpec type,
-                                                        const char *name)
+ASTNode::typecheck_struct_initializers (ref init, TypeSpec type,
+                                        string_view name)
 {
     ASSERT (type.is_structure());
 
@@ -197,16 +196,12 @@ ASTvariable_declaration::typecheck_struct_initializers (ref init, TypeSpec type,
             // Initializer is itself a compound, it ought to be initializing
             // a field that is an array.
             ASTcompound_initializer *cinit = (ASTcompound_initializer *)init.get();
+            ustring fieldname = ustring::format ("%s.%s", name, field.name);
             if (field.type.is_array ()) {
-                ustring fieldname = ustring::format ("%s.%s", name,
-                                                     field.name.c_str());
-                typecheck_initlist (cinit->initlist(),
-                                    field.type, fieldname.c_str());
+                typecheck_initlist (cinit->initlist(), field.type, fieldname);
             } else if (field.type.is_structure()) {
-                ustring fieldname = ustring::format ("%s.%s", name,
-                                                     field.name.c_str());
                 typecheck_struct_initializers (cinit->initlist(), field.type,
-                                               fieldname.c_str());
+                                               fieldname);
             } else {
                 error ("Can't use '{...}' for a struct field that is not an array");
             }
@@ -225,7 +220,7 @@ ASTvariable_declaration::typecheck_struct_initializers (ref init, TypeSpec type,
         if (! assignable(field.type, init->typespec()))
             error ("Can't assign '%s' to '%s %s.%s'",
                    type_c_str(init->typespec()),
-                   type_c_str(field.type), name, field.name.c_str());
+                   type_c_str(field.type), name, field.name);
     }
     return type;
 }
@@ -1117,11 +1112,31 @@ ASTfunction_call::typecheck_builtin_specialcase ()
 
 
 
+TypeSpec
+ASTfunction_call::typecheck_struct_constructor ()
+{
+    StructSpec *structspec = m_sym->typespec().structspec();
+    ASSERT (structspec);
+    m_typespec = m_sym->typespec();
+    if (structspec->numfields() != (int)listlength(args())) {
+        error ("Constructor for '%s' has the wrong number of arguments (expected %d, got %d)",
+               structspec->name(), structspec->numfields(), listlength(args()));
+    }
+    return typecheck_struct_initializers (args(), m_typespec, "this");
+}
+
+
+
 TypeSpec
 ASTfunction_call::typecheck (TypeSpec expected)
 {
     typecheck_children ();
 
+    if (is_struct_ctr()) {
+        // Looks like function call, but is actually struct constructor
+        return typecheck_struct_constructor ();
+    }
+
     bool match = false;
 
     // Look for an exact match, including expected return type

testsuite/oslc-err-struct-ctr/ref/out.txt

@@ -0,0 +1,7 @@
+test.osl:10: error: Cannot initialize structure 'A' with a scalar value
+test.osl:13: error: Constructor for 'rgba' has the wrong number of arguments (expected 2, got 1)
+test.osl:16: error: Constructor for 'rgba' has the wrong number of arguments (expected 2, got 3)
+test.osl:16: error: Too many initializers for 'struct rgba this'
+test.osl:19: error: Can't assign 'string' to 'color this.rgb'
+test.osl:19: error: Can't assign 'string' to 'float this.a'
+FAILED test.osl

testsuite/oslc-err-struct-ctr/run.py

@@ -0,0 +1,5 @@
+#!/usr/bin/env python
+
+# command = oslc("test.osl")
+# don't even need that -- it's automatic
+failureok = 1     # this test is expected to have oslc errors