Closed Bug 766583 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Ms2ger, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

I suggested

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

on IRC... Maybe it'll even work?
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.
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;
}
Actually, I'll probably just do this.
Assignee: nobody → bzbarsky
Whiteboard: [mentor=bz][language=c++][language=python]
Assignee: bzbarsky → nobody
Assignee: nobody → bzbarsky
Whiteboard: [mentor=bz][language=c++][language=python] → [need review]
Ms2ger, feel free to steal the reviews if you want, but I figured you had enough of that on your plate already.
I'm reviewing these
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.
Attachment #741690 - Flags: review?(bugs) → review+
> 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.
Attachment #741691 - Flags: review?(bugs) → review+
Attachment #741692 - Flags: review?(bugs) → review+
Attachment #741693 - Flags: review?(bugs) → review+
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'
Attachment #741694 - Flags: review?(bugs) → review+
Attachment #741695 - Flags: review?(bugs) → review+
Attachment #741696 - Flags: review?(bugs) → review+
Blocks: 865940
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.