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

RESOLVED FIXED in mozilla30

Status

()

Core
Layout
P4
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

3 years ago
Component: CSS Parsing and Computation → Layout
(Assignee)

Comment 1

3 years ago
Created attachment 8361527 [details] [diff] [review]
patch
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8361527 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Blocks: 960899
Priority: -- → P4
Summary: make nsFrameState and enum and consolidate all frame state bit definitions in a single preprocessed file → make nsFrameState an enum and consolidate all frame state bit definitions in a single preprocessed file
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
Attachment #8361527 - Flags: review?(dbaron) → review+
(Assignee)

Comment 3

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3635e6adde70
https://hg.mozilla.org/integration/mozilla-inbound/rev/8600c8ad593f
(Assignee)

Comment 4

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

Comment 5

3 years ago
Created attachment 8362873 [details] [diff] [review]
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.
Attachment #8362873 - Flags: review?(dbaron)
(Assignee)

Comment 6

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d39f73dcfb25
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.
(Assignee)

Comment 8

3 years ago
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 ...
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.
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

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

For the Makefile.in bit.
Attachment #8362873 - Flags: review?(mh+mozilla)
(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.
Attachment #8362873 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

3 years ago
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(...)?
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.
(Assignee)

Comment 15

3 years ago
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 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
Attachment #8362873 - Flags: review?(mh+mozilla)
(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)
(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.
(Assignee)

Comment 19

3 years ago
(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?)
The compiler usually doesn't actually make static consts as actual .rodata. It usually inlines the values.
(Assignee)

Comment 21

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

OK, that simplifies things a lot.
Attachment #8362873 - Attachment is obsolete: true
Attachment #8369942 - Flags: review?(mh+mozilla)
(Assignee)

Comment 22

3 years ago
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 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.
Attachment #8369942 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 24

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8369942 - Flags: review?(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
Attachment #8369942 - Flags: review?(dbaron) → review+
(Assignee)

Comment 26

3 years ago
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.
(Assignee)

Comment 27

3 years ago
In a Windows opt build xul.dll comes out the same size.
(Assignee)

Comment 28

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/b94e05c2de77
https://hg.mozilla.org/mozilla-central/rev/526f189af94e
https://hg.mozilla.org/mozilla-central/rev/64a1fcd74a1b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 1265624
You need to log in before you can comment on or make changes to this bug.