Closed Bug 873368 Opened 7 years ago Closed 6 years ago

generate nsStyleStructList.h

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: heycam, Assigned: heycam)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
nsStyleStructList.h is difficult to edit, when you want to add a new style struct.  This admittedly shouldn't be very often, but I found I had to write a program to generate it with my changes.  So how about we just generate it as part of the build process, to make it easier to add a new style struct in the future?
Attachment #750888 - Flags: review?(dbaron)
Comment on attachment 750888 [details] [diff] [review]
patch

Did you check that what this generates matches the current file?  If so, r=dbaron, but a build peer needs to review as well.

(I kinda wish you'd used python rather than perl, though.)

(A little late, but it might be worth checking if this is even still a measurable performance win.  Compilers might be better these days at switch statements.)
Attachment #750888 - Flags: review?(ted)
Attachment #750888 - Flags: review?(dbaron)
Attachment #750888 - Flags: review+
Attached file output
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> Did you check that what this generates matches the current file?  If so,
> r=dbaron, but a build peer needs to review as well.

It matches except that it generates

  if (...) {
  } else {
    if (...) {
    }
  }

rather than

  if (...) {
  } else if (...) {
  }

in some cases.  It should be equivalent, though.  (See this attachment for the output if you're interested.)

> (I kinda wish you'd used python rather than perl, though.)

I'm not as good at Python. ;_;
Comment on attachment 750888 [details] [diff] [review]
patch

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

From my end I am morally opposed to adding new Perl code to the tree. Sorry :-( This effectively becomes part of the build system and then becomes a burden on the build peers. Python is the build system language of choice these days.

::: layout/style/Makefile.in
@@ +83,5 @@
>  		-I$(srcdir)/../../content/xul/document/src \
>  		$(NULL)
>  
> +nsStyleStructList.h : $(srcdir)/generate-stylestructlist.pl
> +	$(PERL) $< > $@

We really need to figure out a way to express generated files in moz.build world (but that's not your problem).
Attachment #750888 - Flags: review?(ted) → review-
Attached patch patch (rewritten in Python) (obsolete) — Splinter Review
Python version.
Attachment #750888 - Attachment is obsolete: true
Attachment #760167 - Flags: review?(ted)
Attachment #760167 - Flags: review?(dbaron)
Comment on attachment 760167 [details] [diff] [review]
patch (rewritten in Python)

>+// IWYU pragma: private, include "nsStyleStructFwd.h"

What does this mean?

r=dbaron; ted should review too
Attachment #760167 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (in W3C meetings through June 11) (don't cc:, use needinfo? instead) from comment #5)
> Comment on attachment 760167 [details] [diff] [review]
> patch (rewritten in Python)
> 
> >+// IWYU pragma: private, include "nsStyleStructFwd.h"
> 
> What does this mean?

It tells include-what-you-use <http://code.google.com/p/include-what-you-use/> not to include this file directly.
And that annotation was already in the non-generated nsStyleStructList.h so I thought I should preserve it.
Comment on attachment 760167 [details] [diff] [review]
patch (rewritten in Python)

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

This is a little bit hard to review only because I don't fully grok the purpose of the output header, but I trust that dbaron does and his review suffices for that.

Sorry, I've been working on this review on and off for a few days, and I probably spent way too much time on it. Feel free to correct me on anything that doesn't make sense.

::: layout/style/generate-stylestructlist.py
@@ +1,1 @@
> +#!/usr/bin/python

nit: #!/usr/bin/env python

@@ +34,5 @@
> +    "Outline",        "nullptr",
> +    "XUL",            "nullptr",
> +    "SVGReset",       "nullptr",
> +    "Column",         "nullptr",
> +]

I think what you actually want for these data structures is a list of tuples, like:
RESET = [
   ("Background", "nullptr"),
   ...

Then you can just iterate over them like:
for struct, callback in RESET:
  ...

If you really need the list indices you can use enumerate[1] to get them, like:
for i, (struct, callback) in enumerate(RESET):
  ...


It also seems kind of redundant to list "nullptr" here over and over. If you make these tuples you could just omit the second element, but you'd have to handle that specially while enumerating, like so:

RESET = [
   ("Background", ),
   ...

for i, item in enumerate(RESET):
  struct = item[0]
  callback = "nullptr"
  if len(item) == 2:
    callback = item[1]

1. http://docs.python.org/2/library/functions.html#enumerate

@@ +37,5 @@
> +    "Column",         "nullptr",
> +]
> +
> +# How widely to branch on the outermost if statement.
> +TOPLEVELBRANCHES = 4

Is this arbitrary? What effect does changing this have on the generated code?

@@ +50,5 @@
> +    while bits:
> +        if x & ~bits:
> +            return (bits + 1) * 2
> +        bits >>= 1
> +    return 0

You can apparently implement this as:
int(pow(2, math.ceil(math.log(x, 2))))

per:
http://mccormick.cx/news/entries/nearest-power-of-two

@@ +59,5 @@
> +        print "STYLE_STRUCT_INHERITED(%s, %s)" % (INHERITED[j], INHERITED[j + 1])
> +        return
> +    j -= len(INHERITED)
> +    if j < len(RESET):
> +        print "STYLE_STRUCT_RESET(%s, %s)" % (RESET[j], RESET[j + 1])

On further review, you basically just treat these two lists as one long list, right? Why not just put the type into the list, like:
STYLE_STRUCTS = [
("INHERITED", "Font", "CheckFontCallback"),
...
("RESET", "Background", "nullptr"),

Or, to avoid the repetition, something like:
STYLE_STRUCTS = [("INHERITED",) + x for x in [
  ("Font", "CheckFontCallback"),
  ("Color", "CheckColorCallback"),
   ...
   ]] + [("RESET",) + x for x in [
   ("Background", "nullptr"),
   ("Position", "nullptr"),
   ...
   ]]

Then this could just work out to:
print "STYLE_STRUCT_%s(%s, %s)" % STYLE_STRUCTS[i]

@@ +61,5 @@
> +    j -= len(INHERITED)
> +    if j < len(RESET):
> +        print "STYLE_STRUCT_RESET(%s, %s)" % (RESET[j], RESET[j + 1])
> +
> +def printTestTree(min, max, depth, branches):

I'm having a hard time with this function, probably because I don't actually understand the generated code here. Could you add a little bit of documentation?

@@ +88,5 @@
> +                print "  STYLE_STRUCT_TEST_CODE(%s} else if (STYLE_STRUCT_TEST < %d) {)" % (indent, hi)
> +            printTestTree(lo, hi, depth + 1, 2)
> +        print "  STYLE_STRUCT_TEST_CODE(%s})" % indent
> +
> +print """/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */

You should list the name of this Python file here so people know where to look for what generates this.

@@ +128,5 @@
> +// STYLE_STRUCT rather than including the file twice.
> +
> +"""
> +
> +printTestTree(0, nextPowerOf2(count), 0, TOPLEVELBRANCHES)

It might be a little clearer if you put the header and footer strings into named variables and then printed everything in a row, like

HEADER = """
...
"""
FOOTER = """
...
"""

print HEADER
printTestTree(0, nextPowerOf2(count), 0, TOPLEVELBRANCHES)
print FOOTER

Also, is there a reason you need to use nextPowerOf2 here? It looks like you only ever use max as an upper bound on the number of items. Why is using "count" not enough?
Attachment #760167 - Flags: review?(ted)
Attached patch patch (v1.1)Splinter Review
Addressed the other comments, but:

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> I think what you actually want for these data structures is a list of
> tuples, like:
> RESET = [
>    ("Background", "nullptr"),
>    ...
> 
> Then you can just iterate over them like:
> for struct, callback in RESET:
>   ...
> 
> If you really need the list indices you can use enumerate[1] to get them,
> like:
> for i, (struct, callback) in enumerate(RESET):
>   ...

Since we're not enumerating the structs in order, I don't think we can do that.  I've changed to using tuples and a single list, though, since it simplifies the indexing that's done in printEntry.

> It also seems kind of redundant to list "nullptr" here over and over. If you
> make these tuples you could just omit the second element, but you'd have to
> handle that specially while enumerating, like so:
> 
> RESET = [
>    ("Background", ),
>    ...
> 
> for i, item in enumerate(RESET):
>   struct = item[0]
>   callback = "nullptr"
>   if len(item) == 2:
>     callback = item[1]

Checking for length like that seems awkward, especially given how simple printEntry has become.  I've left them as "nullptr".

> > +# How widely to branch on the outermost if statement.
> > +TOPLEVELBRANCHES = 4
> 
> Is this arbitrary? What effect does changing this have on the generated code?

It is what the current, non-generated code is doing.  I mention in the comment why you might want to use a value other than 2 here.

> @@ +50,5 @@
> > +    while bits:
> > +        if x & ~bits:
> > +            return (bits + 1) * 2
> > +        bits >>= 1
> > +    return 0
> 
> You can apparently implement this as:
> int(pow(2, math.ceil(math.log(x, 2))))

OK, I guess that floating point op is probably faster than the loop doing things on integers.

> @@ +61,5 @@
> > +    j -= len(INHERITED)
> > +    if j < len(RESET):
> > +        print "STYLE_STRUCT_RESET(%s, %s)" % (RESET[j], RESET[j + 1])
> > +
> > +def printTestTree(min, max, depth, branches):
> 
> I'm having a hard time with this function, probably because I don't actually
> understand the generated code here. Could you add a little bit of
> documentation?

Added.

> print HEADER
> printTestTree(0, nextPowerOf2(count), 0, TOPLEVELBRANCHES)
> print FOOTER
> 
> Also, is there a reason you need to use nextPowerOf2 here? It looks like you
> only ever use max as an upper bound on the number of items. Why is using
> "count" not enough?

If the number of style structs is not a power of two, you would still get binary-searching nested if statements that work, though it would be a bit wonkier.  I'm not sure if it would be faster or slower.  Doing it this way matches the current non-generated code, though.
Attachment #760167 - Attachment is obsolete: true
Attachment #771898 - Flags: review?(ted)
Comment on attachment 771898 [details] [diff] [review]
patch (v1.1)

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

Thanks for the cleanup and the comments!

::: layout/style/generate-stylestructlist.py
@@ +149,5 @@
> +// nsStyleStructFwd.h that want the structs in id-order just define
> +// STYLE_STRUCT rather than including the file twice.
> +
> +"""
> +FOOTER = """ 

nit: trailing whitespace
Attachment #771898 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/d054daa3cac1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.