Closed
Bug 986492
Opened 10 years ago
Closed 10 years ago
Add fill() convenience function to Codegen.py
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
Attachments
(2 files, 4 obsolete files)
16.22 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
396.67 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee: general → jorendorff
Attachment #8394852 -
Flags: review?(peterv)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
Making it official.
Attachment #8394976 -
Attachment is obsolete: true
Attachment #8396309 -
Flags: review?(peterv)
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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!
Assignee | ||
Comment 9•10 years ago
|
||
Updated patch.
Attachment #8396309 -
Attachment is obsolete: true
Attachment #8396309 -
Flags: review?(peterv)
Attachment #8400070 -
Flags: review?(peterv)
Assignee | ||
Comment 10•10 years ago
|
||
There were a few trivial changes after all.
Attachment #8394852 -
Attachment is obsolete: true
Attachment #8394852 -
Flags: review?(peterv)
Attachment #8400071 -
Flags: review?(peterv)
Comment 11•10 years ago
|
||
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?
Attachment #8400071 -
Flags: review?(peterv) → review+
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0a8ae91b6d9e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Comment 13•10 years ago
|
||
One more patch here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [keep-open]
Assignee | ||
Comment 14•10 years ago
|
||
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).
Assignee | ||
Comment 15•10 years ago
|
||
peterv: Review ping. :) :)
Assignee | ||
Comment 16•10 years ago
|
||
Rebased.
Attachment #8400070 -
Attachment is obsolete: true
Attachment #8400070 -
Flags: review?(peterv)
Assignee | ||
Comment 17•10 years ago
|
||
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.
Attachment #8406278 -
Flags: review?(peterv)
Assignee | ||
Comment 18•10 years ago
|
||
Nope, rebasing across this is a huge pain.
Comment 19•10 years ago
|
||
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•10 years ago
|
||
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)
Attachment #8400070 -
Attachment is obsolete: false
Updated•10 years ago
|
Attachment #8400070 -
Attachment is obsolete: true
Comment 21•10 years ago
|
||
Comment on attachment 8406278 [details] [diff] [review] bug-986492-use-fill-everywhere-v4.patch Review of attachment 8406278 [details] [diff] [review]: ----------------------------------------------------------------- See comment 20.
Attachment #8406278 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(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.
Assignee | ||
Comment 23•10 years ago
|
||
(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.
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0436eec4d294 https://hg.mozilla.org/integration/mozilla-inbound/rev/d6064bbed583
Assignee | ||
Updated•10 years ago
|
Whiteboard: [keep-open]
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0436eec4d294 https://hg.mozilla.org/mozilla-central/rev/d6064bbed583
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•