Last Comment Bug 960848 - make nsFrameState an 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 ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
P4 normal (vote)
: mozilla30
Assigned To: Cameron McCormack (:heycam)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 1265624
Blocks: 960899
  Show dependency treegraph
 
Reported: 2014-01-16 18:19 PST by Cameron McCormack (:heycam)
Modified: 2016-04-18 22:47 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (89.36 KB, patch)
2014-01-16 23:26 PST, Cameron McCormack (:heycam)
dbaron: review+
Details | Diff | Splinter Review
Part 3: Generate a header containing frame state bit macros for VS2010 (4.44 KB, patch)
2014-01-21 01:49 PST, Cameron McCormack (:heycam)
dbaron: review+
Details | Diff | Splinter Review
Part 3: Generate consts rather than enum values for nsFrameState bits for VS2010. (2.10 KB, patch)
2014-02-04 02:02 PST, Cameron McCormack (:heycam)
dbaron: review+
mh+mozilla: feedback+
Details | Diff | Splinter Review

Description User image Cameron McCormack (:heycam) 2014-01-16 18:19:07 PST
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 User image Cameron McCormack (:heycam) 2014-01-16 23:26:42 PST
Created attachment 8361527 [details] [diff] [review]
patch
Comment 2 User image David Baron :dbaron: ⌚️UTC-8 2014-01-17 17:10:36 PST
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 4 User image Cameron McCormack (:heycam) 2014-01-18 00:04:36 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/334a63e35b74 for lots of red on platforms I didn't try.
Comment 5 User image Cameron McCormack (:heycam) 2014-01-21 01:49:45 PST
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.
Comment 6 User image Cameron McCormack (:heycam) 2014-01-21 01:49:53 PST
https://tbpl.mozilla.org/?tree=Try&rev=d39f73dcfb25
Comment 7 User image Seth Fowler [:seth] [:s2h] 2014-01-21 14:41:00 PST
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 User image Cameron McCormack (:heycam) 2014-01-21 14:57:28 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-01-21 15:40:58 PST
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 User image Cameron McCormack (:heycam) 2014-01-21 15:44:23 PST
(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 User image Cameron McCormack (:heycam) 2014-01-21 15:49:49 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-01-21 15:51:41 PST
(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 User image Cameron McCormack (:heycam) 2014-01-21 15:53:16 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-01-21 16:09:40 PST
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 User image Cameron McCormack (:heycam) 2014-01-21 16:12:17 PST
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 User image Mike Hommey [:glandium] 2014-01-27 20:02:55 PST
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 User image Mike Hommey [:glandium] 2014-01-27 20:04:33 PST
(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 User image David Baron :dbaron: ⌚️UTC-8 2014-01-27 22:12:13 PST
(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 User image Cameron McCormack (:heycam) 2014-02-03 15:56:58 PST
(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 User image Mike Hommey [:glandium] 2014-02-03 17:09:30 PST
The compiler usually doesn't actually make static consts as actual .rodata. It usually inlines the values.
Comment 21 User image Cameron McCormack (:heycam) 2014-02-04 02:02:39 PST
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.
Comment 22 User image Cameron McCormack (:heycam) 2014-02-04 02:04:07 PST
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 User image Mike Hommey [:glandium] 2014-02-04 03:07:08 PST
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 User image Cameron McCormack (:heycam) 2014-02-04 13:01:17 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-02-04 15:26:26 PST
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 User image Cameron McCormack (:heycam) 2014-02-04 15:28:59 PST
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 User image Cameron McCormack (:heycam) 2014-02-04 17:59:01 PST
In a Windows opt build xul.dll comes out the same size.

Note You need to log in before you can comment on or make changes to this bug.