766583 - Try to get rid of all the unholy const_casting in the new bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I suggested

const T& MakeThisConst(T& foo) { return foo; }

on IRC... Maybe it'll even work?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I tried sticking a Constify() as above around all arguments, and pretty immediately found that this doesn't quite do what we want.  In particular, it will constify non-nullable interface arguments.

So we really do need to only constify for some arguments but not others.  Presumably the getJSToNative.... code will need to add another something to the return tuple.

Comment 2

[Boris Zbarsky [:bzbarsky]]

For anyone who wants to try, what I actually used was:

// Inline helper to constify types we want to constify
template<typename T>
const T& Constify(const T& t)
{
  return t;
}

Comment 3

[Boris Zbarsky [:bzbarsky]]

Actually, I'll probably just do this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop declaring dictionaries as const on the stack in bindings code.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop declaring sequences as const on the stack in bindings code.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop declaring nullable things as const on the stack in bindings code.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop declaring strings as const on the stack in bindings code.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Stop declaring optional arguments as const on the stack in bindings code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Stop declaring unions as const on the stack in bindings code.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Stop declaring variadic arguments as const on the stack in bindings code.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ms2ger, feel free to steal the reviews if you want, but I figured you had enough of that on your plate already.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I'm reviewing these

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 741690 [details] [diff] [review]
part 1.  Stop declaring dictionaries as const on the stack in bindings code.

>         selfRef = "${declName}"
> 
>         declType = CGGeneric(actualTypeName)
> 
>-        # If we're a member of something else, the const
>-        # will come from the Optional or our container.
>-        if not isMember:
>-            declType = CGWrapper(declType, pre="const ")
>-            selfRef = "const_cast<%s&>(%s)" % (typeName, selfRef)
>-
>         # We do manual default value handling here, because we
>         # actually do want a jsval, and we only handle null anyway
>         # NOTE: if isNullOrUndefined or isDefinitelyObject are true,
>         # we know we have a value, so we don't have to worry about the
>         # default value.
>         if (not isNullOrUndefined and not isDefinitelyObject and
>             defaultValue is not None):
>             assert(isinstance(defaultValue, IDLNullValue))
>@@ -3228,19 +3222,19 @@ for (uint32_t i = 0; i < length; ++i) {
>             # Check that the value we have can in fact be converted to
>             # a dictionary, and return failureCode if not.
>             template = CGIfWrapper(
>                 CGGeneric(failureCode),
>                 "!IsConvertibleToDictionary(cx, &${val}.toObject())").define() + "\n\n"
>         else:
>             template = ""
> 
>-        template += ("if (!%s.Init(cx, %s)) {\n"
>+        template += ("if (!${declName}.Init(cx, %s)) {\n"
>                      "%s\n"
>-                     "}" % (selfRef, val, exceptionCodeIndented.define()))
>+                     "}" % (val, exceptionCodeIndented.define()))
So selfRef becomes unused, right? If so, it can be removed.


>@@ -3981,17 +3975,25 @@ class CGCallGenerator(CGThing):
>                                                                descriptorProvider,
>                                                                resultAlreadyAddRefed)
> 
>         args = CGList([CGGeneric(arg) for arg in argsPre], ", ")
>         for (a, name) in arguments:
>             # This is a workaround for a bug in Apple's clang.
>             if a.type.isObject() and not a.type.nullable() and not a.optional:
>                 name = "(JSObject&)" + name
>-            args.append(CGGeneric(name))
>+            arg = CGGeneric(name)
>+            # Now constify the things that need it
>+            def needsConst(a):
>+                if a.type.isDictionary():
>+                    return True
>+                return False
>+            if needsConst(a):
>+                arg = CGWrapper(arg, pre="Constify(", post=")")
>+            args.append(arg)

I assume some other patch will add stuff to needsConst. Otherwise it is rather useless.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> So selfRef becomes unused, right? If so, it can be removed.

Yes, indeed.

> I assume some other patch will add stuff to needsConst.

Each patch in this bug adds stuff to it, yeah, except the variadic one.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 741694 [details] [diff] [review]
part 5.  Stop declaring optional arguments as const on the stack in bindings code.

>@@ -3974,16 +3965,18 @@ class CGCallGenerator(CGThing):
>                 if a.type.isDictionary():
>                     return True
>                 if a.type.isSequence():
>                     return True
>                 if a.type.nullable():
>                     return True
>                 if a.type.isString():
>                     return True
>+                if a.optional and not a.defaultValue:
>+                    return True
Could you add a comment why ' and not a.defaultValue'

Comment 16

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d52e50616251
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7f89b3429b
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f4449bddbbfa
   https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf01da4f4f7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/22fc8e985a12
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6268bbee606e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c6da9de6c807

with the comment.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d52e50616251
https://hg.mozilla.org/mozilla-central/rev/3e7f89b3429b
https://hg.mozilla.org/mozilla-central/rev/f4449bddbbfa
https://hg.mozilla.org/mozilla-central/rev/aaf01da4f4f7
https://hg.mozilla.org/mozilla-central/rev/22fc8e985a12
https://hg.mozilla.org/mozilla-central/rev/6268bbee606e
https://hg.mozilla.org/mozilla-central/rev/c6da9de6c807