On Wed, Nov 11, 2015 at 5:07 PM, L. David Baron <[email protected]> wrote: > Jesse suggested using enum classes for the values of enumerated CSS > properties, and I tend to agree that this is a good idea. It gives > both stricter typechecking than the current uint8_t (etc.) values, > and better warnings for switch() statements omitting values. > > I decided to try this out for box-sizing to see what it would look > like. Patches are linked from: > https://bugzilla.mozilla.org/show_bug.cgi?id=1223653#c1 > > Interestingly, the typechecking actually caught a real bug, where we > were passing an nscoord (size inside the box-sizing) to a function > that expected a uint8_t (enumerated value of box-sizing property), > because we actually intended to call a different function. > > But I'm mainly interested in feedback on: > > 1. Naming > ========= > > Do the names I chose seem reasonable? I replaced: > -#define NS_STYLE_BOX_SIZING_CONTENT 0 > -#define NS_STYLE_BOX_SIZING_PADDING 1 > -#define NS_STYLE_BOX_SIZING_BORDER 2 > with: > +namespace mozilla { > + > +enum class StyleBoxSizing : uint8_t { > + Content, > + Padding, > + Border > +}; > + > +} // namespace mozilla > so that most caller changes look like: > - case NS_STYLE_BOX_SIZING_BORDER: > + case StyleBoxSizing::Border: > or like: > if (parent && > - parent->StylePosition()->mBoxSizing != NS_STYLE_BOX_SIZING_BORDER) { > + parent->StylePosition()->mBoxSizing != StyleBoxSizing::Border) {
The names make sense to me. > 2. Casts > ======== > > Does anybody see a good way to eliminate the casts I needed to use > in the main patch (described in the patch header)? We can probably make them templates? > 3. General > ========== > > Do others agree this is a good idea? And are people ok with it > being done gradually? Yeah, I think this is a great idea. We should benefit more from the type system. Given you are doing this, I'd like to try this approach in my new patches as well. One more thing may need to be done before more conversion comes: we need a typesafe way to declare bitmasks which we are using for several properties. I filed bug 1217241 for something similiar several weeks ago. - Xidorn _______________________________________________ dev-tech-layout mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-layout
