960848 - make nsFrameState an enum and consolidate all frame state bit definitions in a single preprocessed file

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Cameron McCormack (:heycam)]

Having frame state bits defined using macro calls will make it easier to write some debugging functions that list the named bit values.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 2

[David Baron :dbaron:]

Comment on attachment 8361527 [details] [diff] [review]
patch

Could you merge the "public state bits" and "private state bits"
sections for block frames, and just list them all together, in numeric
order?

(Maybe stick a patch in your stack before this one to swap the values
of NS_BLOCK_HAS_FIRST_LETTER_CHILD and NS_BLOCK_FRAME_HAS_INSIDE_BULLET
to keep the two NS_BLOCK_FRAME_HAS_*_BULLET flags together numerically?)


>+#ifndef nsFrameState_h___
>+#define nsFrameState_h___
...
>+#endif /* nsFrameState_h___ */ 

Please use one trailing underscore instead of three.

>+#define NS_FRAME_STATE_BIT(n_) (nsFrameState(1ULL << (n_)))

Please use nsFrameState_size_t(1) << (n_) rather than assuming ULL
is a uint64_t.  (And move the line below the typedef, too.)


Could you fix the 80th-column violations in the comments describing
the nsTextFrame bits?


(Maybe also add assertions to nsTableColFrame::SetColType and
nsTableColGroupFrame::SetColType that the type is currently 0?  The
current code is pretty dangerous.  Or maybe even another bug
to fix?)


Also, please email dev-tech-layout saying you've done this.

r=dbaron

Comment 3

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3635e6adde70
https://hg.mozilla.org/integration/mozilla-inbound/rev/8600c8ad593f

Comment 4

[Cameron McCormack (:heycam)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/334a63e35b74 for lots of red on platforms I didn't try.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 3: Generate a header containing frame state bit macros for VS2010

It turns out VS2010 has some odd problems with sized enums, so I've had to keep #defines for that compiler.  Since you can't generate #defines with macros, I've got a small python script that generates them from nsFrameStateBits.h.

Comment 6

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=d39f73dcfb25

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8362873 [details] [diff] [review]
Part 3: Generate a header containing frame state bit macros for VS2010

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

::: layout/generic/nsFrameState.h
@@ +21,5 @@
> + * Visual Studio 2010 has trouble with the sized enum.  Although sized enums
> + * are supported, two problems arise:
> + *
> + *   1. Implicit conversions from the enum type to the equivalently sized
> + *      integer size are not performed, leading to many compile errors.

I'm actually surprised this worked anywhere! Disallowing such implicit conversions was one of the motivations for the C++11 strongly typed enums.

Comment 8

[Cameron McCormack (:heycam)]

I'm not using strongly typed enums, only enums with a specified storage size, i.e. I'm doing

  enum Blah : uint64_t { ... };

rather than

  enum class Blah ...

Comment 9

[David Baron :dbaron:]

Can you not get away with putting the values in an unnamed, untyped enum for VS2010?  (Along with the typedef that you have.)

I guess I'm ok with this, though I'd probably have preferred the python script be a little pickier about the input format and give errors when it hits something unexpected, and you'll need build peer review for the makefile changes.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #9)
> Can you not get away with putting the values in an unnamed, untyped enum for
> VS2010?  (Along with the typedef that you have.)

I did try this, and ran into the same odd test failures.  I suspect the compiler is doing something not quite right with enum values >= 2**32, but that's just a guess.

> I guess I'm ok with this, though I'd probably have preferred the python
> script be a little pickier about the input format and give errors when it
> hits something unexpected, and you'll need build peer review for the
> makefile changes.

I figured that I didn't need to be so picky, given that mistakes in nsFrameStateBits.h will get caught on other platforms.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8362873 [details] [diff] [review]
Part 3: Generate a header containing frame state bit macros for VS2010

For the Makefile.in bit.

Comment 12

[David Baron :dbaron:]

(In reply to Cameron McCormack (:heycam) from comment #10)
> > I guess I'm ok with this, though I'd probably have preferred the python
> > script be a little pickier about the input format and give errors when it
> > hits something unexpected, and you'll need build peer review for the
> > makefile changes.
> 
> I figured that I didn't need to be so picky, given that mistakes in
> nsFrameStateBits.h will get caught on other platforms.

I was more worried about things that might create differences between platforms.

Comment 13

[Cameron McCormack (:heycam)]

Do you have a suggestion for how to tighten it up?  Make the regex only accept identifier-like things in the first/third arguments of FRAME_STATE_BIT(...)?

Comment 14

[David Baron :dbaron:]

I was worried about things like:
 * the macro being split over multiple lines
 * comments being inserted where you didn't expect them
leading to the confusing result of build failures on VS2010 because the value just didn't get defined.  It would be preferable if the error happened in the generation script so that it was clear why it was happening.

Comment 15

[Cameron McCormack (:heycam)]

Ah OK.  I'll add some error reporting in the Python script if it encounters FRAME_STATE_BITs that it would ignore due to not matching the whole regex.  And maybe I'll handle comments a bit better.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8362873 [details] [diff] [review]
Part 3: Generate a header containing frame state bit macros for VS2010

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

If you want something more robust, generate headers from json instead of generating one header from another. See for example what is done for histograms in toolkit/components/telemetry.

That being said, sure, you can't do
  #define FRAME_STATE_BIT(foo, bar, baz) #define baz bar
but you can do
  #define FRAME_STATE_BIT(foo, bar, baz) static const baz = bar;

Why not just do that (or something alike)?

(Also you're not using FRAME_STATE_GROUP and the first arg to FRAME_STATE_BIT)

::: layout/generic/generate-framestatebitmacros.py
@@ +15,5 @@
> +import re
> +import sys
> +
> +if len(sys.argv) != 2:
> +    print >>sys.stdrr, 'syntax: %s path/to/nsFrameStateBits.h' % sys.argv[0]

stderr

@@ +21,5 @@
> +
> +input_file = sys.argv[1]
> +line_num = 0
> +pp_pattern = re.compile('^\s*#')
> +bit_pattern = re.compile('^\s*FRAME_STATE_BIT\(\s*[^,]*,\s*(\d+)\s*,\s*([^,)]*)\s*\)')

Using group names ( (?P<Foo>pattern) ) would probably help.

@@ +23,5 @@
> +line_num = 0
> +pp_pattern = re.compile('^\s*#')
> +bit_pattern = re.compile('^\s*FRAME_STATE_BIT\(\s*[^,]*,\s*(\d+)\s*,\s*([^,)]*)\s*\)')
> +with open(input_file, 'r') as f:
> +    line_num = line_num + 1

You're not using this variable.

::: layout/generic/nsFrameState.h
@@ +27,5 @@
> + *      test failures result, which might well be due to a compiler bug.
> + *
> + * So with VS2010 we use macros for the state bits and forgo the increased
> + * type safety of the enum.  nsFrameStateBitMacros.h is generated by
> + * generate-framestatebitmacros.h, which takes nsFrameStateBits.h as input.

generate-framestatebitmacros.py

Comment 17

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #16)
>   #define FRAME_STATE_BIT(foo, bar, baz) static const baz = bar;

(missing a type, obviously, but you get the point)

Comment 18

[David Baron :dbaron:]

(In reply to Mike Hommey [:glandium] from comment #16)
> If you want something more robust, generate headers from json instead of
> generating one header from another. See for example what is done for
> histograms in toolkit/components/telemetry.

Given that this is for a non-current version of a single compiler, I'd prefer not to.

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Mike Hommey [:glandium] from comment #16)
> That being said, sure, you can't do
>   #define FRAME_STATE_BIT(foo, bar, baz) #define baz bar
> but you can do
>   #define FRAME_STATE_BIT(foo, bar, baz) static const baz = bar;
> 
> Why not just do that (or something alike)?

Will that not result in wasted memory for these constants?  (And if it does, is that acceptable?)

Comment 20

[Mike Hommey [:glandium]]

The compiler usually doesn't actually make static consts as actual .rodata. It usually inlines the values.

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Part 3: Generate consts rather than enum values for nsFrameState bits for VS2010.

OK, that simplifies things a lot.

Comment 22

[Cameron McCormack (:heycam)]

Oh, the frame state bit groups are used in bug 960899.  They probably should have been added in that bug, but I'm a bit lazy to extricate them now.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8369942 [details] [diff] [review]
Part 3: Generate consts rather than enum values for nsFrameState bits for VS2010.

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

::: layout/generic/nsFrameState.h
@@ +31,5 @@
> +
> +typedef nsFrameState_size_t nsFrameState;
> +
> +#define FRAME_STATE_BIT(group_, value_, name_) \
> +const nsFrameState name_ = NS_FRAME_STATE_BIT(value_);

*static* const.

Note this needs double checking that MSVC does the "right" thing.

Comment 24

[Cameron McCormack (:heycam)]

Was looking up how consts work and in C++ they automatically have internal linkage without having to use "static" (and which you override with "extern" if you wanted).  But I can include it if it's clearer.

Comment 25

[David Baron :dbaron:]

Comment on attachment 8369942 [details] [diff] [review]
Part 3: Generate consts rather than enum values for nsFrameState bits for VS2010.

Using static for constants in a header seems likely to generate a lot of warnings about statics that are unused.  So probably best as it is, as long as you've checked it doesn't have any effect on the binary's size.

r=dbaron

Comment 26

[Cameron McCormack (:heycam)]

As I understand it, "static const" is exactly the same as "const" at namespace scope.  I will check the binary size / presence of symbols for the constants.

Comment 27

[Cameron McCormack (:heycam)]

In a Windows opt build xul.dll comes out the same size.

Comment 28

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b94e05c2de77
https://hg.mozilla.org/integration/mozilla-inbound/rev/526f189af94e
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a1fcd74a1b

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b94e05c2de77
https://hg.mozilla.org/mozilla-central/rev/526f189af94e
https://hg.mozilla.org/mozilla-central/rev/64a1fcd74a1b