From 2db1ce72f0d2713d8f332514afd1ba8061cf3926 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 13:13:24 +0200 Subject: [PATCH 1/7] Remove references to unused substitution variables in getJSToNativeConversionTemplate. --- .../dom/bindings/codegen/CodegenRust.py | 27 +------------------ 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 8c58f669b54..02e7071158d 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -281,7 +281,6 @@ class CGMethodCall(CGThing): isDefinitelyObject=True), { "declName" : "arg%d" % distinguishingIndex, - "holderName" : ("arg%d" % distinguishingIndex) + "_holder", "val" : distinguishingArg }) @@ -449,9 +448,6 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, substitution performed on it as follows: ${val} replaced by an expression for the JS::Value in question - ${valPtr} is a pointer to the JS::Value in question - ${holderName} replaced by the holder's name, if any - ${declName} replaced by the declaration's name ${haveValue} replaced by an expression that evaluates to a boolean for whether we have a JS::Value. Only used when defaultValue is not None. @@ -460,15 +456,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, (declType). This is allowed to be None if the conversion code is supposed to be used as-is. 3) A boolean indicating whether the caller has to do optional-argument handling. - This will only be true if isOptional is true and if the returned template - expects both declType and holderType to be wrapped in Optional<>, with - ${declName} and ${holderName} adjusted to point to the Value() of the - Optional, and Construct() calls to be made on the Optional<>s as needed. - ${declName} must be in scope before the generated code is entered. - - If holderType is not None then ${holderName} must be in scope - before the generated code is entered. """ # If we have a defaultValue then we're not actually optional for # purposes of what we need to be declared as. @@ -945,13 +933,10 @@ class CGArgumentConverter(CGThing): } self.replacementVariables = { "declName" : "arg%d" % index, - "holderName" : ("arg%d" % index) + "_holder" } self.replacementVariables["val"] = string.Template( "(*${argv}.offset(${index}))" ).substitute(replacer) - self.replacementVariables["valPtr"] = ( - "&" + self.replacementVariables["val"]) if argument.defaultValue: self.replacementVariables["haveValue"] = string.Template( "${index} < ${argc}").substitute(replacer) @@ -2763,8 +2748,6 @@ def getUnionTypeTemplateVars(type, descriptorProvider): assert not type.isObject() jsConversion = string.Template(template).substitute({ "val": "value", - "valPtr": None, - "holderName": None, }) jsConversion = CGWrapper(CGGeneric(jsConversion), pre="Ok(Some(", post="))") @@ -3413,9 +3396,7 @@ class CGProxySpecialOperation(CGPerSignatureCall): treatNullAs=argument.treatNullAs) templateValues = { "declName": argument.identifier.name, - "holderName": argument.identifier.name + "_holder", "val": "(*desc).value", - "valPtr": "&(*desc).value" } self.cgRoot.prepend(instantiateJSToNativeConversionTemplate(template, templateValues)) elif operation.isGetter(): @@ -4400,7 +4381,7 @@ class CGNativeMember(ClassMethod): None if isMember is true. The third element is a template for actually returning a value stored in - "${declName}" and "${holderName}". This means actually returning it if + "${declName}". This means actually returning it if we're not outparam, else assigning to the "retval" outparam. If isMember is true, this can be None, since in that case the caller will never examine this value. @@ -4922,13 +4903,7 @@ class CallbackMember(CGNativeMember): def getResultConversion(self): replacements = { "val": "rval", - "mutableVal": "&rval", - "holderName" : "rvalHolder", "declName" : "rvalDecl", - # We actually want to pass in a null scope object here, because - # wrapping things into our current compartment (that of mCallback) - # is what we want. - "obj": "nullptr" } if isJSImplementedDescriptor(self.descriptorProvider): From e53c768b9ef765700d40fc153b295d7630f30baf Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 13:40:02 +0200 Subject: [PATCH 2/7] Use CGIfWrapper in instantiateJSToNativeConversionTemplate. --- .../script/dom/bindings/codegen/CodegenRust.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 02e7071158d..0a0eb3b19eb 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -868,19 +868,8 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, post=";") if argcAndIndex is not None: - declConstruct = None - holderConstruct = None - - conversion = CGList( - [CGGeneric( - string.Template("if ${index} < ${argc} {").substitute( - argcAndIndex - )), - declConstruct, - holderConstruct, - CGIndenter(conversion), - CGGeneric("}")], - "\n") + condition = string.Template("${index} < ${argc}").substitute(argcAndIndex) + conversion = CGIfWrapper(conversion, condition) result.append(conversion) # Add an empty CGGeneric to get an extra newline after the argument From 455465f3c62a3e7a545920c6284589f0661317ac Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 14:30:02 +0200 Subject: [PATCH 3/7] Move the handling of default values in getJSToNativeConversionTemplate into handleOptional. This is a first step towards moving responsibility for default values into the caller, which should improve their code (in particular for the dictionary codegen. This also introduces a Dictionary::empty function, whose implementation is a hack. The implementation will be improved once its codegen has access to the raw default values. Besides dictionaries, this commit does not change the generated code. --- .../dom/bindings/codegen/CodegenRust.py | 124 +++++++----------- 1 file changed, 50 insertions(+), 74 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 0a0eb3b19eb..cf7b1908206 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -474,7 +474,8 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, needsRooting = typeNeedsRooting(type, descriptorProvider) - def handleOptional(template, declType, isOptional): + def handleOptional(template, declType, isOptional, default): + assert (defaultValue is None) == (default is None) if isOptional: template = "Some(%s)" % template declType = CGWrapper(declType, pre="Option<", post=">") @@ -482,6 +483,11 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, else: initialValue = None + if default is not None: + template = CGIfElseWrapper("${haveValue}", + CGGeneric(template), + CGGeneric(default)).define() + return (template, declType, isOptional, initialValue, needsRooting) # Unfortunately, .capitalize() on a string will lowercase things inside the @@ -509,23 +515,17 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, exceptionCode))), post="\n") - # A helper function for handling default values. Takes a template - # body and the C++ code to set the default value and wraps the - # given template body in handling for the default value. - def handleDefault(template, setDefault): + # A helper function for handling null default values. Checks that the + # default value, if it exists, is null. + def handleDefaultNull(nullValue): if defaultValue is None: - return template - return CGIfElseWrapper("${haveValue}", - CGGeneric(template), - CGGeneric(setDefault)).define() + return None - # A helper function for handling null default values. Much like - # handleDefault, but checks that the default value, if it exists, is null. - def handleDefaultNull(template, codeToSetNull): - if (defaultValue is not None and - not isinstance(defaultValue, IDLNullValue)): + if not isinstance(defaultValue, IDLNullValue): raise TypeError("Can't handle non-null default value here") - return handleDefault(template, codeToSetNull) + + assert type.nullable() or type.isAny() or type.isDictionary() + return nullValue # A helper function for wrapping up the template body for # possibly-nullable objecty stuff @@ -545,10 +545,6 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, "} else {\n" + CGIndenter(onFailureNotAnObject(failureCode)).define() + "}\n") - if type.nullable(): - templateBody = handleDefaultNull(templateBody, "None") - else: - assert(defaultValue is None) return templateBody @@ -569,15 +565,12 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, if type.nullable(): declType = CGWrapper(declType, pre="Option<", post=" >") - templateBody = CGGeneric("match FromJSValConvertible::from_jsval(cx, ${val}, ()) {\n" - " Ok(value) => value,\n" - " Err(()) => { %s },\n" - "}" % exceptionCode) + templateBody = ("match FromJSValConvertible::from_jsval(cx, ${val}, ()) {\n" + " Ok(value) => value,\n" + " Err(()) => { %s },\n" + "}" % exceptionCode) - templateBody = handleDefaultNull(templateBody.define(), - "None") - - return handleOptional(templateBody, declType, isOptional) + return handleOptional(templateBody, declType, isOptional, handleDefaultNull("None")) if type.isGeckoInterface(): assert not isEnforceRange and not isClamp @@ -592,7 +585,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, template = wrapObjectTemplate(conversion, isDefinitelyObject, type, failureCode) - return handleOptional(template, declType, isOptional) + return handleOptional(template, declType, isOptional, handleDefaultNull("None")) descriptorType = descriptor.memberType if isMember else descriptor.nativeType @@ -622,7 +615,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, templateBody = wrapObjectTemplate(templateBody, isDefinitelyObject, type, failureCode) - return handleOptional(templateBody, declType, isOptional) + return handleOptional(templateBody, declType, isOptional, handleDefaultNull("None")) if type.isSpiderMonkeyInterface(): raise TypeError("Can't handle SpiderMonkey interface arguments yet") @@ -641,20 +634,19 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, else: nullBehavior = treatAs[treatNullAs] - def getConversionCode(): - conversionCode = ( - "match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n" - " Ok(strval) => strval,\n" - " Err(_) => { %s },\n" - "}" % (nullBehavior, exceptionCode)) - - if defaultValue is None: - return conversionCode - - if isinstance(defaultValue, IDLNullValue): - assert(type.nullable()) - return handleDefault(conversionCode, "None") + conversionCode = ( + "match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n" + " Ok(strval) => strval,\n" + " Err(_) => { %s },\n" + "}" % (nullBehavior, exceptionCode)) + if defaultValue is None: + default = None + elif isinstance(defaultValue, IDLNullValue): + assert type.nullable() + default = "None" + else: + assert defaultValue.type.tag() == IDLType.Tags.domstring value = "str::from_utf8(data).unwrap().to_owned()" if type.nullable(): value = "Some(%s)" % value @@ -666,13 +658,11 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, ", ".join(["'" + char + "' as u8" for char in defaultValue.value] + ["0"]), value)) - return handleDefault(conversionCode, default) - declType = "DOMString" if type.nullable(): declType = "Option<%s>" % declType - return handleOptional(getConversionCode(), CGGeneric(declType), isOptional) + return handleOptional(conversionCode, CGGeneric(declType), isOptional, default) if type.isByteString(): assert not isEnforceRange and not isClamp @@ -686,11 +676,8 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, declType = CGGeneric("ByteString") if type.nullable(): declType = CGWrapper(declType, pre="Option<", post=">") - conversionCode = handleDefaultNull(conversionCode, "None") - else: - assert defaultValue is None - return handleOptional(conversionCode, declType, isOptional) + return handleOptional(conversionCode, declType, isOptional, handleDefaultNull("None")) if type.isEnum(): assert not isEnforceRange and not isClamp @@ -718,12 +705,11 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, if defaultValue is not None: assert(defaultValue.type.tag() == IDLType.Tags.domstring) - template = handleDefault(template, - ("%sValues::%s" % - (enum, - getEnumValueName(defaultValue.value)))) + default = "%sValues::%s" % (enum, getEnumValueName(defaultValue.value)) + else: + default = None - return handleOptional(template, CGGeneric(enum), isOptional) + return handleOptional(template, CGGeneric(enum), isOptional, default) if type.isCallback(): assert not isEnforceRange and not isClamp @@ -749,8 +735,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, assert not isEnforceRange and not isClamp declType = CGGeneric("JSVal") - templateBody = handleDefaultNull("${val}", "NullValue()") - return handleOptional(templateBody, declType, isOptional) + return handleOptional("${val}", declType, isOptional, handleDefaultNull("NullValue()")) if type.isObject(): raise TypeError("Can't handle object arguments yet") @@ -765,23 +750,13 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, assert not isOptional typeName = CGDictionary.makeDictionaryName(type.inner) - declType = CGGeneric(typeName) - - # We do manual default value handling here, because we - # actually do want a jsval, and we only handle null anyway - if defaultValue is not None: - assert(isinstance(defaultValue, IDLNullValue)) - val = "if ${haveValue} { ${val} } else { NullValue() }" - else: - val = "${val}" - - template = ("match %s::new(cx, %s) {\n" + template = ("match %s::new(cx, ${val}) {\n" " Ok(dictionary) => dictionary,\n" " Err(_) => return 0,\n" - "}" % (typeName, val)) + "}" % typeName) - return handleOptional(template, declType, isOptional) + return handleOptional(template, declType, isOptional, handleDefaultNull("%s::empty()" % typeName)) if type.isVoid(): assert not isOptional @@ -822,12 +797,10 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, if type.nullable(): defaultStr = "Some(%s)" % defaultStr + else: + defaultStr = None - template = CGIfElseWrapper("${haveValue}", - CGGeneric(template), - CGGeneric(defaultStr)).define() - - return handleOptional(template, declType, isOptional) + return handleOptional(template, declType, isOptional, defaultStr) def instantiateJSToNativeConversionTemplate(templateTuple, replacements, argcAndIndex=None): @@ -4070,6 +4043,9 @@ class CGDictionary(CGThing): return string.Template( "impl<'a, 'b> ${selfName}<'a, 'b> {\n" + " pub fn empty() -> ${selfName} {\n" + " ${selfName}::new(ptr::null(), NullValue()).unwrap()\n" + " }\n" " pub fn new(cx: *JSContext, val: JSVal) -> Result<${selfName}, ()> {\n" " let object = if val.is_null_or_undefined() {\n" " ptr::null()\n" From cb2723c4ed4c6539b33029d9ecc3460914fb26a8 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 14:46:36 +0200 Subject: [PATCH 4/7] Remove getJSToNativeConversionTemplate's initialValue return value. This commit does not change the generated code. --- .../dom/bindings/codegen/CodegenRust.py | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index cf7b1908206..7225b8f0ffa 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -479,16 +479,13 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, if isOptional: template = "Some(%s)" % template declType = CGWrapper(declType, pre="Option<", post=">") - initialValue = "None" - else: - initialValue = None if default is not None: template = CGIfElseWrapper("${haveValue}", CGGeneric(template), CGGeneric(default)).define() - return (template, declType, isOptional, initialValue, needsRooting) + return (template, declType, isOptional, needsRooting) # Unfortunately, .capitalize() on a string will lowercase things inside the # string, which we do not want. @@ -762,7 +759,7 @@ def getJSToNativeConversionTemplate(type, descriptorProvider, failureCode=None, assert not isOptional # This one only happens for return values, and its easy: Just # ignore the jsval. - return ("", None, False, None, False) + return ("", None, False, False) if not type.isPrimitive(): raise TypeError("Need conversion for argument type '%s'" % str(type)) @@ -813,7 +810,7 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, replace ${argc} and ${index}, where ${index} is the index of this argument (0-based) and ${argc} is the total number of arguments. """ - (templateBody, declType, dealWithOptional, initialValue, needsRooting) = templateTuple + (templateBody, declType, dealWithOptional, needsRooting) = templateTuple if dealWithOptional and argcAndIndex is None: raise TypeError("Have to deal with optional things, but don't know how") @@ -832,8 +829,8 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, CGGeneric(replacements["declName"]), CGGeneric(": "), declType] - if initialValue: - newDecl.append(CGGeneric(" = " + initialValue)) + if dealWithOptional: + newDecl.append(CGGeneric(" = None")) newDecl.append(CGGeneric(";")) result.append(CGList(newDecl)) conversion = CGWrapper(conversion, @@ -2702,7 +2699,7 @@ def getUnionTypeTemplateVars(type, descriptorProvider): name = type.name typeName = "/*" + type.name + "*/" - (template, _, _, _, _) = getJSToNativeConversionTemplate( + (template, _, _, _) = getJSToNativeConversionTemplate( type, descriptorProvider, failureCode="return Ok(None);", exceptionCode='return Err(());', isDefinitelyObject=True, isOptional=False) @@ -4083,15 +4080,13 @@ class CGDictionary(CGThing): return "/* uh oh */ %s" % name def getMemberType(self, memberInfo): - (member, (templateBody, declType, - dealWithOptional, initialValue, _)) = memberInfo + (member, (templateBody, declType, dealWithOptional, _)) = memberInfo if dealWithOptional: declType = CGWrapper(declType, pre="Optional< ", post=" >") return declType.define() def getMemberConversion(self, memberInfo): - (member, (templateBody, declType, - dealWithOptional, initialValue, _)) = memberInfo + (member, (templateBody, declType, dealWithOptional, _)) = memberInfo replacements = { "val": "value.unwrap()" } if member.defaultValue: replacements["haveValue"] = "value.is_some()" From d14efebb5c2ad4dcfa178cfc140bc54f81d93b2d Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 14:58:49 +0200 Subject: [PATCH 5/7] Move the assignment of 'None' in the no-argument-passed case into an else branch. This is the only case where we assign into an argument local twice, so removing it will allow us to make that binding immutable. --- src/components/script/dom/bindings/codegen/CodegenRust.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 7225b8f0ffa..327fa119c9f 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -829,8 +829,6 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, CGGeneric(replacements["declName"]), CGGeneric(": "), declType] - if dealWithOptional: - newDecl.append(CGGeneric(" = None")) newDecl.append(CGGeneric(";")) result.append(CGList(newDecl)) conversion = CGWrapper(conversion, @@ -839,7 +837,9 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, if argcAndIndex is not None: condition = string.Template("${index} < ${argc}").substitute(argcAndIndex) - conversion = CGIfWrapper(conversion, condition) + conversion = CGIfElseWrapper(condition, + conversion, + CGGeneric("%s = None" % replacements["declName"])) result.append(conversion) # Add an empty CGGeneric to get an extra newline after the argument From aed95dfd941d418faa1f816895e097fe867c2ac3 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 15:04:25 +0200 Subject: [PATCH 6/7] Move the assignment outside the if when dealing with optional arguments. --- .../script/dom/bindings/codegen/CodegenRust.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 327fa119c9f..7bb5663b026 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -824,6 +824,12 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, string.Template(templateBody).substitute(replacements) ) + if argcAndIndex is not None: + condition = string.Template("${index} < ${argc}").substitute(argcAndIndex) + conversion = CGIfElseWrapper(condition, + conversion, + CGGeneric("None")) + if declType is not None: newDecl = [CGGeneric("let mut "), CGGeneric(replacements["declName"]), @@ -835,12 +841,6 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, pre="%s = " % replacements["declName"], post=";") - if argcAndIndex is not None: - condition = string.Template("${index} < ${argc}").substitute(argcAndIndex) - conversion = CGIfElseWrapper(condition, - conversion, - CGGeneric("%s = None" % replacements["declName"])) - result.append(conversion) # Add an empty CGGeneric to get an extra newline after the argument # conversion. From 262dc30c18700ea9da026041d937bb217283edf6 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 May 2014 15:18:53 +0200 Subject: [PATCH 7/7] Assign into the argument binding directly in instantiateJSToNativeConversionTemplate. --- .../dom/bindings/codegen/CodegenRust.py | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py index 7bb5663b026..a6c984d2e02 100644 --- a/src/components/script/dom/bindings/codegen/CodegenRust.py +++ b/src/components/script/dom/bindings/codegen/CodegenRust.py @@ -831,17 +831,19 @@ def instantiateJSToNativeConversionTemplate(templateTuple, replacements, CGGeneric("None")) if declType is not None: - newDecl = [CGGeneric("let mut "), - CGGeneric(replacements["declName"]), - CGGeneric(": "), - declType] - newDecl.append(CGGeneric(";")) + newDecl = [ + CGGeneric("let mut "), + CGGeneric(replacements["declName"]), + CGGeneric(": "), + declType, + CGGeneric(" = "), + conversion, + CGGeneric(";"), + ] result.append(CGList(newDecl)) - conversion = CGWrapper(conversion, - pre="%s = " % replacements["declName"], - post=";") + else: + result.append(conversion) - result.append(conversion) # Add an empty CGGeneric to get an extra newline after the argument # conversion. result.append(CGGeneric(""))