986492 - Add fill() convenience function to Codegen.py

Status: RESOLVED FIXED

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

Description

[Jason Orendorff [:jorendorff]]

Codegen.py has a lot of multiline strings of C++ right in the middle of Python code. This is just the nature of what the program does.

The way it is right now, this C++ code requires completely different indentation from the surrounding Python, which makes the Python really hard to read.

I've got a way around this; it takes up more vertical space but I think it's a pretty huge improvement. Patch coming.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: bug-986492-Codegen-fill-v1.patch

Here's the patch to add the fill() function, and a few examples to show the cost and benefit of using it. Mainly vertical space for readability.

(I found it so compelling that I've actually converted the whole file; I've got a full patch which I'll also post FYI. Of course it would be super disruptive to just land this in the middle of ongoing work, so we can put it off until a convenient time, or never.)

To confirm that this patch does not change the output, I did this:

  cd $OBJDIR
  cp -R dom/bindings dom/bindings-before
  hg qpush
  make -C dom/bindings/ AbstractWorkerBinding.cpp
  diff -U3 -r --exclude '*.json' --exclude '*.o' \
       dom/bindings-before dom/bindings

Comment 2

[Jason Orendorff [:jorendorff]]

Oh -- that patch has this odd comment in it, in two places:
    # BOGUS extra newline at the top
Here the script is generating a blank line right at the top of a C++ block, which is poor style.
Instead of fixing it, I commented it.

My full patch found many other bogus blank lines and such. It seemed best to keep the output completely unchanged, so I just commented them. Easy to fix later.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: bug-986492-use-fill-everywhere-v1.patch

The rest of the patch.

The existing code has some rather subtle conventions on which strings should have newlines at the end. Statements don't, even multiline statements. Declarations mostly do. Function definitions, I think, always do? It's kind of a mess, though; I don't think the rules are always followed. There are unintentional blank lines in the output.

So... I changed the rule. With this patch, a string that's multiple lines of C++, or a full line with semicolon, always has a newline at the end.

Unfortunately this means a lot of churn. :-\

Comment 4

[Boris Zbarsky [:bzbarsky]]

The existing code's handling of newlines is pretty bad.  Not least because it's a mix of styles.

What I've generally been aiming for in newer code is that any given string being passed around should not contain leading/trailing newlines, and that things including that string are responsible for adding newlines around it as needed.  But making that play nice with existing code is a bit fun.

Comment 5

[Jason Orendorff [:jorendorff]]

Yes - either way would have worked, I just wanted something consistent.

For hard-to-articulate aesthetic reasons I like "whole lines" better. "Line Line Line" is a simpler grammar than "Line Line LineWithoutTrailingNL". Concatenation (the most common operation) is a simpler with whole lines. Shrug.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: bug-986492-use-fill-everywhere-v2.patch

Making it official.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It really depends on the context.  When building up the if condition in a case like this:

  if (foo ||
      bar ||
      baz) 

you really don't want a trailing newline on the "baz".  For most other cases, trailing newline seems ok.

Comment 8

[Jason Orendorff [:jorendorff]]

Right, with the patch, Python strings for C++ expressions never end in a newline (and seldom contain a newline at all); Python strings for C++ statements, comments, declarations, definitions, etc. always do.

In one or two places, we have
    if (${condition})
with
    condition=" || \n    ".join(parts)
which seems nice enough.

There's also a place where we have
    code = "if (x) {\n  statements;\n}\n"
and we want to add an else clause. The newline is in the way. So:
    code = code.lstrip() + " else {\n  more statements;\n}\n"
That it looks like a hack is a feature!

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: bug-986492-use-fill-everywhere-v3.patch

Updated patch.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: bug-986492-Codegen-fill-v2.patch

There were a few trivial changes after all.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8400071 [details] [diff] [review]
bug-986492-Codegen-fill-v2.patch

Review of attachment 8400071 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +107,5 @@
> +    # string.Template.
> +    multiline_substitution_re = re.compile(r"( *)\$\*{(\w+)}(\n)?")
> +
> +    def replace(match):
> +        """ Replaces a line like '  $*{xyz}\n' with '${xyz_2}'. """

Maybe "... with '${xyz_n}' with n the depth" or something like it? And shouldn't it mention that it adds it to args?

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0a8ae91b6d9e

Comment 13

[Jason Orendorff [:jorendorff]]

One more patch here.

Comment 14

[Jason Orendorff [:jorendorff]]

peterv: Review ping. :) I know I told you just yesterday there's no rush, but now I remember this is blocking bug 547140 which blocks some work Eric Faust is doing on proxies and assignment (I forget the bug number, I think it's a dup of bug 979671).

Comment 15

[Jason Orendorff [:jorendorff]]

peterv: Review ping. :) :)

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: bug-986492-use-fill-everywhere-v4.patch

Rebased.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8406278 [details] [diff] [review]
bug-986492-use-fill-everywhere-v4.patch

Request flag was accidentally reset. Argh.

I'll start rebasing stuff around this tomorrow morning.

Comment 18

[Jason Orendorff [:jorendorff]]

Nope, rebasing across this is a huge pain.

Comment 19

[Peter Van der Beken [:peterv]]

I finished reviewing v3 on the plane yesterday. I'll try to look at an interdiff with v4 I guess.

What is the plan for all the "# BOGUS" stuff? I don't mind checking them in for now, but if these really are bogus we should fix them.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 8400070 [details] [diff] [review]
bug-986492-use-fill-everywhere-v3.patch

Review of attachment 8400070 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +278,5 @@
> +              ${constructorID},
> +              ${parentHooks}
> +            } };
> +            """,
> +            **locals())

Not too fond of using locals().

@@ +745,3 @@
>      def __init__(self, child, condition):
> +        CGList.__init__(self, [
> +            CGGeneric("if (" + condition + ") {\n"),

This isn't exactly equivalent though, if condition is multi-line it won't be lined up with the opening parens. Or do we somehow not have multi-line conditions anymore?

@@ +752,5 @@
>  
>  class CGIfElseWrapper(CGList):
>      def __init__(self, condition, ifTrue, ifFalse):
> +        CGList.__init__(self, [
> +            CGGeneric("if (" + condition + ") {\n"),

Same here.

@@ +1297,5 @@
>              return self._define(True)
>          return "%s%s%s(%s);\n" % (self._template(), self._decorators(), self.name, self._argstring(True))
>  
>      def _define(self, fromDeclare=False):
> +        return self.definition_prologue(fromDeclare) + self.definition_body() + self.definition_epilogue()

I wonder if it wouldn't make more sense to just indent self.definition_body() here, instead of in all the implementations.

@@ +1620,5 @@
>  
>      def generate_code(self):
> +        # BOGUS extra blank line at start of function
> +        header = dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +1655,5 @@
> +                    nativeType=self.descriptor.nativeType,
> +                    name=self.descriptor.interface.identifier.name))
> +
> +        hasInstanceCode = dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +2528,5 @@
>  
>      def definition_body(self):
> +        # BOGUS extra blank line at start of method
> +        return indent(dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +2548,5 @@
>  
>      def definition_body(self):
> +        # BOGUS extra blank line at start of method
> +        return indent(dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +2822,5 @@
> +
> +            ${parent}
> +            ${unforgeable}
> +              aCache->SetWrapper(obj);
> +            $*{slots}

I'd make AssertInheritanceChain/CreateBindingJSObject/InitUnforgeableProperties/InitMemberSlots not indent and line these $ up with the rest.

@@ +2876,5 @@
> +              }
> +
> +            ${global_}
> +            ${unforgeable}
> +            $*{slots}

Same here.

@@ +2919,5 @@
> +                                                     Class.ToJSClass(),
> +                                                     aOptions,
> +                                                     aPrincipal);
> +
> +            ${unforgeable}

And here.

@@ +3160,5 @@
>                  "SelfRef objRef;\n"
>                  "JS::Rooted<JS::Value> val(cx, JS::ObjectValue(*${source}));\n" +
>                  xpconnectUnwrap +
>                  "if (NS_FAILED(rv)) {\n"
> +                "${indentedCodeOnFailure}"

Could use fill with $*{codeOnFailure}?

@@ +3192,5 @@
> +                """,
> +                exceptionCode=exceptionCode,
> +                **self.substitution)
> +        else:
> +            self.substitution["codeOnFailure"] = codeOnFailure

We don't really need this else anymore, right? Might as well drop it.

@@ +3532,5 @@
>              # We'll get traced by the sequence or dictionary or union tracer
>              declType = CGGeneric("JSObject*")
>              declArgs = None
> +        templateBody = "${declName} = &${val}.toObject();\n"
> +        setToNullCode = "${declName} = nullptr;;\n"  # BOGUS extra semicolon

Please remove the extra semicolon.

@@ +4305,5 @@
>  
>          if defaultValue is not None:
>              if isinstance(defaultValue, IDLNullValue):
>                  assert type.nullable()
> +                template = handleDefault(template, setNull.rstrip() + ";\n")  # BOGUS extra semicolon

Please remove that semicolon.

@@ +4554,5 @@
> +            nonFiniteCode = lenientFloatCode
> +        else:
> +            nonFiniteCode = ('ThrowErrorMessage(cx, MSG_NOT_FINITE, "%s");\n'
> +                             "%s" % (firstCap(sourceDescription), exceptionCode))
> +        template = template.rstrip()

This is one example that I personally don't like, having to strip newlines because we want to append an else :-(.

@@ +8622,5 @@
> +              // Since we're dealing with an Xray, do the resolve on the
> +              // underlying object first.  That gives it a chance to
> +              // define properties on the actual object as needed, and
> +              // then use the fact that it created the objects as a flag
> +              // o avoid re-resolving the properties if someone deletes

s/ o / to /

@@ +9307,5 @@
> +        else:
> +            delete += "\n"  # BOGUS extra blank line
> +
> +        delete += dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +9326,5 @@
>      def getBody(self):
>          # Per spec, we do indices, then named props, then everything else
>          if self.descriptor.supportsIndexedProperties():
> +            addIndices = dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +9530,5 @@
> +                }
> +                """,
> +                callGetter=(CGProxyIndexedGetter(self.descriptor, templateValues).define() %
> +                            self.descriptor.nativeType),
> +                expando=getUnforgeableOrExpando)

I'd rename expando to unforgeableOrExpando.

@@ +11625,5 @@
>  
>          if self.refcounted:
>              if self.parentIface:
> +                ccImpl = dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +11635,5 @@
> +
> +                    """)
> +            else:
> +                ccImpl = dedent("""\
> +

Is this backslash unnecessary if you remove a blank line?

@@ +12596,5 @@
> +              return${errorReturn};
> +            }
> +            """,
> +            errorReturn=self.getDefaultRetval(),
> +            methodName=self.methodName)

Reorder the arguments.

@@ +12671,5 @@
> +            }
> +            """,
> +            errorReturn=self.getDefaultRetval(),
> +            attrName=self.descriptorProvider.binaryNames.get(self.attrName,
> +                                                             self.attrName))

Reorder the arguments.

@@ +13278,5 @@
> +            NS_IMPL_ADDREF_INHERITED(${nativeType}, ${parentType})
> +            NS_IMPL_RELEASE_INHERITED(${nativeType}, ${parentType})
> +
> +            NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(${nativeType}, ${parentType})
> +            ${traverse}NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Couldn't this be $*{traverse} on its own line? (Same for trace and unlink below)

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 8406278 [details] [diff] [review]
bug-986492-use-fill-everywhere-v4.patch

Review of attachment 8406278 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 20.

Comment 22

[Jason Orendorff [:jorendorff]]

(In reply to Peter Van der Beken [:peterv] from comment #20)
> Not too fond of using locals().

OK. Expanded it to keyword arguments like everyplace else.

> @@ +745,3 @@
> >      def __init__(self, child, condition):
> > +        CGList.__init__(self, [
> > +            CGGeneric("if (" + condition + ") {\n"),
> 
> This isn't exactly equivalent though, if condition is multi-line it won't be
> lined up with the opening parens. Or do we somehow not have multi-line
> conditions anymore?

Apparently not, but I changed it back to a CGWrapper with reindent=True anyway.

> @@ +1297,5 @@
> >              return self._define(True)
> >          return "%s%s%s(%s);\n" % (self._template(), self._decorators(), self.name, self._argstring(True))
> >  
> >      def _define(self, fromDeclare=False):
> > +        return self.definition_prologue(fromDeclare) + self.definition_body() + self.definition_epilogue()
> 
> I wonder if it wouldn't make more sense to just indent
> self.definition_body() here, instead of in all the implementations.

I think so. Filed follow-up bug 998080.

> > +        header = dedent("""\
> Is this backslash unnecessary if you remove a blank line?

OK, I changed dedent() to remove a blank line, instead of only fill() doing it. This removed all the backslashes on dedent() calls.

> > +            ${parent}
> > +            ${unforgeable}
> > +              aCache->SetWrapper(obj);
> > +            $*{slots}
> 
> I'd make
> AssertInheritanceChain/CreateBindingJSObject/InitUnforgeableProperties/
> InitMemberSlots not indent and line these $ up with the rest.

Done.

> >                  xpconnectUnwrap +
> >                  "if (NS_FAILED(rv)) {\n"
> > +                "${indentedCodeOnFailure}"
> 
> Could use fill with $*{codeOnFailure}?

Probably - filed follow-up bug 998091.

> > +        else:
> > +            self.substitution["codeOnFailure"] = codeOnFailure
> 
> We don't really need this else anymore, right? Might as well drop it.

Don't we need it in the __str__ method?

This class is pretty weird.

> Please remove the extra semicolon.

Done in a second changeset (because I really want to land most of the work in a clean changeset that produces exactly the same output, byte for byte).

> @@ +4554,5 @@
> > +            nonFiniteCode = lenientFloatCode
> > +        else:
> > +            nonFiniteCode = ('ThrowErrorMessage(cx, MSG_NOT_FINITE, "%s");\n'
> > +                             "%s" % (firstCap(sourceDescription), exceptionCode))
> > +        template = template.rstrip()
> 
> This is one example that I personally don't like, having to strip newlines
> because we want to append an else :-(.

Well, that's how you know a convention is working: it makes hacks look like hacks. ;-)

> > +              // o avoid re-resolving the properties if someone deletes
> 
> s/ o / to /

Done in the second changeset.

> > +                expando=getUnforgeableOrExpando)
> 
> I'd rename expando to unforgeableOrExpando.

Done.


> > +              return${errorReturn};
> > +            }
> > +            """,
> > +            errorReturn=self.getDefaultRetval(),
> > +            methodName=self.methodName)
> 
> Reorder the arguments.

Done.

> > +            ${traverse}NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> Couldn't this be $*{traverse} on its own line? (Same for trace and unlink
> below)

Done.

Comment 23

[Jason Orendorff [:jorendorff]]

(In reply to Peter Van der Beken [:peterv] from comment #19)
> I finished reviewing v3 on the plane yesterday. I'll try to look at an
> interdiff with v4 I guess.

It was just unbitrotting really.

> What is the plan for all the "# BOGUS" stuff? I don't mind checking them in
> for now, but if these really are bogus we should fix them.

Filed follow-up bug 998115.

Comment 24

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0436eec4d294
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6064bbed583

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0436eec4d294
https://hg.mozilla.org/mozilla-central/rev/d6064bbed583