Closed Bug 960848 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files, 1 obsolete file)

Having frame state bits defined using macro calls will make it easier to write some debugging functions that list the named bit values.
Component: CSS Parsing and Computation → Layout
Attached patch patchSplinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8361527 - Flags: review?(dbaron)
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+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/334a63e35b74 for lots of red on platforms I didn't try.
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)
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.
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.
(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 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.
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.
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.
(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.
OK, that simplifies things a lot.
Attachment #8362873 - Attachment is obsolete: true
Attachment #8369942 - Flags: review?(mh+mozilla)
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+
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.
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+
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.
In a Windows opt build xul.dll comes out the same size.
Depends on: 1265624
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: