Closed Bug 817194 Opened 7 years ago Closed 6 years ago
Don't require to append "Initializer" to dictionary return value types
See bug 816187 comment 2: > [The Initializer idiom is] what makes sure the dictionary is initialized properly so that callees only have to set the non-default values in it. It doesn't make me happy, because Nullable<WebGLContextAttributesInitializer> is annoying... the other option is to rejigger things in codegen so that we can actually initialize the return value in some way before calling the method, not just declare it. Maybe that's the way to go; let me know if you want me to give that a stab. Note that we could live with the ugly for now and do the "initialize the return value" as a followup.
So what's really going on here is that we're trying to optimize things. In particular, for dictionary _arguments_ we handle default values in the dictionary by only assigning them if there is no value in the given object. But that means that the dictionary default constructor (which is triggered by declaring the argument conversion bits for a dictionary) doesn't initialize the default values. So we're using this Initializer() thing to invoke a non-default constructor which _does_ initialize them. We could just give up on that and just have the default constructor initialize the default values. For numbers this is not a problem, really; it's mostly a waste for strings, which we'll end up doing possibly-unnecessary allocations for... The other option is to use a non-default constructor of some sort (which doesn't initialize the default values) for argument conversions. For pure dictionary arguments this works ok, because there are no optional or nullable dictionaries, so there are no complications in terms of calling the constructor. But for sequences of dictionaries and the like, things get hard, because sequences very much like invoking default constructors. A third option is to add an explicit method that sets the default values. But then C++ code that wants to return dictionaries would have to remember to call this method if it's not setting all the values, which seems very error-prone... Any other bright suggestions?
I thought I could get rid of it by simply initializing the return value, which would work ok for dictionary and nullable dictionary return values. But we'd still have a problem for return values that are a sequence of dictionaries. :(
Whiteboard: [need review]
This is great.
Attachment #812679 - Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa0cbe4a54d No need for docs; the "Initializer" bit wasn't documented. :(
And https://hg.mozilla.org/integration/mozilla-inbound/rev/ccedb8b1cefc because someone added another *Initializer use.
Whiteboard: [need review]
Target Milestone: --- → mozilla27
Unfortunately still failing: https://tbpl.mozilla.org/php/getParsedLog.php?id=28881485&tree=Mozilla-Inbound Backout: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a535608cea81 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c062bb25ff57
That's build system shenanigans, looks like. I filed bug 924992.
Relanded with a clobber: https://hg.mozilla.org/integration/mozilla-inbound/rev/04d774d42e1b
And https://hg.mozilla.org/integration/mozilla-inbound/rev/036ac2eefb65 due to MSVC's buggy namespace/template handling.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
bug 924724, which is koi+, depends on the change here. Need to uplift this patch
blocking-b2g: --- → koi?
(In reply to C.J. Ku[:CJKu] from comment #12) > bug 924724, which is koi+, depends on the change here. Need to uplift this > patch Do we still need this on b2g26 given the branch patches in bug 924724?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13) > (In reply to C.J. Ku[:CJKu] from comment #12) > > bug 924724, which is koi+, depends on the change here. Need to uplift this > > patch > > Do we still need this on b2g26 given the branch patches in bug 924724? Hi Ryan, No, to limit code change, I will merge the change into one patch for b2g26 and mozilla_beta
FWIW, anything that lands on m-b will be merged to b2g26 automatically. So unless there's a big reason to make separate patches, I wouldn't worry about it. Just make a beta patch and get approval to land it and the rest will sort itself out :)
Based on comment 14, it sounds like this doesn't block after all?
blocking-b2g: koi+ → koi?
Given comment 14 and 16, removing the blocking flag here.
blocking-b2g: koi? → -
You need to log in before you can comment on or make changes to this bug.