Comment on attachment 793327 [details] [diff] [review] Patch It looks like this IDL: void foo(optional (object or long)? arg = 5); will fail with this patch because nullable will be true, so will defaultValue and then we'll hit: assert(isinstance(defaultValue, IDLNullValue)) So the code that handles the "nullable with default value" situation needs to be adjusted.... and perhaps more codegen tests added? >+ valueMissing = "!(${haveValue}) || " This is unused in your defaultValue branch, no? >+ if tag in numericSuffixes: I'm not a huge fan of the code-duplication here for the defaultStr computations (wrt the other places we have default values). Could we maybe factor that out into a helper function called from here and from the isDOMString() and isPrimitive() cases? That method could also assert that the type is one of numeric, bool, DOMString. Note that if you do it that way, the code here can look identical for numeric and bool. What happens for string default values if the union contains an enum? Followup is fine if this is too annoying to handle, but I'd like us to make sure it fails codegen, not C++ compilation. >+ templateBody = CGWrapper(CGIndenter(templateBody), pre="{\n", post="\n}") >+ templateBody = CGList([null, templateBody], " else ") I'd prefer CGIfElseWrapper here. That's what it's for. Also, "null" is the wrong name for that bit. Should probably be called "handleDefault" instead? >+ return IDLValue(self.location, subtype, returnType.value) Worth documenting why we're using subtype, not returnType.type here. And returnType is more like coercedValue, not returnType, right? >+++ b/dom/bindings/test/TestCodeGen.webidl Please add to the other two test webidls as well so they stay in sync.

(In reply to Boris Zbarsky [:bz] from comment #2) > Comment on attachment 793327 [details] [diff] [review] > Patch > > It looks like this IDL: > > void foo(optional (object or long)? arg = 5); > > will fail with this patch because nullable will be true, so will > defaultValue and then we'll hit: > > assert(isinstance(defaultValue, IDLNullValue)) > > So the code that handles the "nullable with default value" situation needs > to be adjusted.... and perhaps more codegen tests added? > Yeah, nullable unions were pretty broken... > > What happens for string default values if the union contains an enum? > Followup is fine if this is too annoying to handle, but I'd like us to make > sure it fails codegen, not C++ compilation. > WebIDL.WebIDLError: error: Cannot coerce type String to type DoubleOrTestEnum. I don't think it should be too annoying, but I'd rather leave it for a followup.

Let me know if you want to see some generated code

Why, for a nullable union, does the new coercion code just try to coerce to the member types? Does that make |void foo((object or long)? = null)| work, and if so how?

(In reply to Boris Zbarsky [:bz] from comment #5) > Why, for a nullable union, does the new coercion code just try to coerce to > the member types? Does that make |void foo((object or long)? = null)| work, > and if so how? Well, that obviously doesn't work but | void foo(optional (object or long)? foopy = null); | does. I think the reason that typechecks is because we call IDLNullValue.coerceToType which is happy because it sees that we're assigning to a nullable union.

Oh, I see, because tht's an IDLNullValue, not an IDLValue. I see, thanks. And the point is that if we took the normal nullable() codepath the type would end up wrong, because we would return a value with type "type" as opposed to the thing we really want to return? I guess now I'm confused by how |optional long? a = 5| works... OK, I'll sort through this.

(In reply to Boris Zbarsky [:bz] from comment #7) > Oh, I see, because tht's an IDLNullValue, not an IDLValue. I see, thanks. > And the point is that if we took the normal nullable() codepath the type > would end up wrong, because we would return a value with type "type" as > opposed to the thing we really want to return? > Exactly. > I guess now I'm confused by how |optional long? a = 5| works... OK, I'll > sort through this. It checks the inner type: http://mxr.mozilla.org/mozilla-central/source/dom/bindings/parser/WebIDL.py#2394

I meant more in terms of the IDLValue in that case ending up with a "nullable long" type as opposed to a "long" type. But it looks like consumers don't care enough about the precise difference between those two so it kinda ends up working out.

Comment on attachment 794873 [details] [diff] [review] Interdiff The code duplication for the string case is still not that great. :( >+ default = CGGeneric("// We handled this case right before this block.") Can we just make the condition above this be "if defaultValue and not isinstance(defaultValue, IDLNullValue)" instead? >+""" %s >+%s.SetStringData(data, ArrayLength(data) - 1);""" % (defaultStr, > unionArgumentObj)) Drop the space before that first %s. >+ templateBody = handleNull(templateBody, declLoc,) Drop the trailing comma, please. >+ if defaultValue and nullable: This seems kinda weird to me. It'll construct the holder in cases when there is no defaultValue and we're nullable... Wouldn't it be better to remove the "elif nullable" bit from the "if defaultValue" chunk and then make the part after the above line look like this: if nullable: if defaultValue: if isinstance(defaultValue, IDLNullValue): extraConditionForNull = "!(${haveValue}) ||" else: extraConditionForNull = "${haveValue} &&" else: extraConditionForNull = "" templateBody = handleNull(templateBody, declLoc, extraConditionForNull=extraConditionForNull) or so? This will ensure that we don't construct the holder in the case when we're nullable and the value is null. >+ # We must first check for unions to ensure that we handle nullable >+ # unions propertly. How about: # We must first check for unions to ensure that even if the union is # nullable we end up with the right flat member type, not the union's type. ? r=me with the above fixed.

Any news here? The patch provided seems bitrotted.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #11) > Any news here? The patch provided seems bitrotted. I just moved so I still don't have relibale internet access. I'll fix this up and push it when I get a chance.

David, want me to just drive this in?

Sure, if you could. Thanks!