This patch ensures that, when nsIntMargin is not just a typedef for nsMargin (i.e. when NSCOORD_IS_FLOAT is defined) it nonetheless exposes all the same method names, by factoring out all of the real code into a template base class.

for clarity's sake: comment 1 is due to a typo, bug 470545 is actually a duplicate of bug 470546.

I've updated this for the changes in bug 448830 and also extended it to nsPoint, nsRect, and nsSize. Will attach revised patch shortly.

and here it is. Switching review request from vlad to roc because roc did bug 448830.

+ nsMargin(nsTMargin<nscoord> const& o) : nsTMargin<nscoord>(o) {} Why do we have to define a conversion constructor from the parent type? Might as well make the base type TMargin in the 'mozilla' namespace.

(In reply to comment #5) > + nsMargin(nsTMargin<nscoord> const& o) : nsTMargin<nscoord>(o) {} > Why do we have to define a conversion constructor from the parent type? The overloaded operators in nsTMargin return the parent type, which is not assignment-compatible with the subclass unless you define this constructor. > Might as well make the base type TMargin in the 'mozilla' namespace. Will do Monday. (I assume also for the other base types.)

All four template classes (TPoint, TSize, TMargin, TRect) are now in namespace mozilla, without the 'ns' prefix. The concrete classes remain in the default namespace and ns-prefixed. We can't move them without extensive changes to other files, which I think should be done in a separate, as-mechanical-as-possible patch after this lands. In this case we should maybe also consider unifying these classes with the similar, but API-incompatible, gfx{Point,Size,Rect} classes...

I need to talk to Vlad about converging these types. I'd like to converge on an API where everything's functional, i.e. everything that changes the 'this' rect returns a new rect instead.

+// These are subclasses instead of typedefs for compatibility with +// forward struct declarations elsewhere, and so that nsMargin and +// nsIntMargin are distinct types even when NS_COORD_IS_FLOAT is not +// defined. This means we have to define all the constructors again, +// and define a conversion constructor from the parent type. Actually what we can do is add an unused int parameter to the TMargin/TPoint/TSize/TRect template, then we can have as many different units as we want. We can add to ToNearest/ToInside/ToOutside/ToAppUnits methods to *all* TRect types, I think that's fine even though they're only useful for a subset. Then we don't need new classes, just typedefs.

(In reply to comment #8) > I need to talk to Vlad about converging these types. I'd like to converge on an > API where everything's functional, i.e. everything that changes the 'this' rect > returns a new rect instead. Sounds good to me. (In reply to comment #9) > +// These are subclasses instead of typedefs for compatibility with > +// forward struct declarations elsewhere, and so that nsMargin and > +// nsIntMargin are distinct types even when NS_COORD_IS_FLOAT is not > +// defined. > Actually what we can do is add an unused int parameter to the > TMargin/TPoint/TSize/TRect template, then we can have as many different units > as we want. ... > Then we don't need new classes, just typedefs. Alas, no; typedefs will conflict with struct nsMargin; in other headers. AFAIK there is no way to forward-declare a typedef.

I like the current patch. Only changes I would make are to move more of the nsRect functions inline, and move ScaleRoundOut to be on nsRect only. Also, I know the STL uses the typedef own_type thing a lot, but I find it confusing...

We could replace those forward declarations with includes of the right headers. There aren't too many of them. But maybe it's not worth it.

I think I'd prefer TMargin instead of own_type ... what do you think Zack?

I don't much care what we *call* the typedef, but I want one; I find it much easier to get the function prototypes right that way. I wouldn't mind replacing the forward declarations with #includes, maybe even in this patch; on the other hand, it could be done as a separate patch that might be easier to review. Your call.

(In reply to comment #8) > I need to talk to Vlad about converging these types. I'd like to converge on an > API where everything's functional, i.e. everything that changes the 'this' rect > returns a new rect instead. I'm maybe irrationally worried about temporary/stack size/etc. usage; but it seems like you really want both options available, with the methods that modify 'this' having a consistent naming convention. e.g., expanding a rect by some amount is pretty common.

We seem to have agreement on the work in this patch, so I'm tagging it for checkin; follow-up bugs 491589, 491591, 491593 filed.

Patch seems to have conflicts on trunk, can you refresh it?

Here's an updated patch. Unfortunately, I had to kludge around this build error: layout/svg/base/src/nsSVGIntegrationUtils.cpp:309: error: ‘struct mozilla::TRect<int>’ has no member named ‘ToOutsidePixels’ The only way I know to define member functions of a template class for some but not all values of the template parameter is really ugly. (You write a stub definition in the base template that deliberately triggers a compile error if instantiated, and then you override that with specializations in the cases that you want to work. And now I think about it, it might only work for free template functions, not members of a template class.) I did not do this in this revision of the patch. Please let me know if you want me to do it and/or have a better idea.

It occurred to me this morning that we *have* to find some way of pulling the conversion methods into the template base class if we want to switch the current subclasses to typedefs. Also, on reflection, I don't like the way the v3a status quo exposes the fact that nsRect isn't really what you get back from the arithmetic operators. And the ugliness I was complaining about yesterday is limited to the header file, although it does leak out as inelegant error messages. So I tried to make that change. Unfortunately, the trick I know for providing a template function only if specialized turns out not to work for member functions of a class template. You have a definition in the base template for each method that might be available if specialized, with a deliberate instantiation-time error in it. But then when, in nsRect.cpp, we explicitly instantiate the class template, that error fires. So this patch as coded does not compile. Other than that, it does the job - everything *else* compiles and svg/base/src/nsSVGIntegrationUtils.cpp no longer needs modification. Is there a template wizard in the house?

I should add that there are two modifications I can think of that might work but both are suboptimal. We could leave the conversion methods as undefined extern functions in the base template; this is what I did to verify that the code worked apart from the problem described above. This would make misuse of the conversion methods be only a link-time error; also I think the compiler would be within its rights to object to the missing definition when the template is explicitly instantiated. Or we could provide additional specializations of the functions in nsRect.cpp to mask the error. These would presumably abort if ever called. The problem with that is, if we ever do whole-program optimization, the compiler would be within its rights to detect these specializations and suppress the errors that were the whole point of this exercise!

I never got back around to this and roc's doing a superset of it in bug 641426. No reason to keep this bug hanging around.