470506 - Template base classes for Point, Size, Margin, Rect

Status: RESOLVED DUPLICATE of bug 641426

(Core :: Graphics, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

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.

Comment 2

[Zack Weinberg (:zwol)]

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

Comment 3

[Zack Weinberg (:zwol)]

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

Comment 4

[Zack Weinberg (:zwol)]

Attachment: v2: applies to latest trunk, does all four of Point, Size, Margin, Rect

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 6

[Zack Weinberg (:zwol)]

(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.)

Comment 7

[Zack Weinberg (:zwol)]

Attachment: v3: template classes now in namespace mozilla

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...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+// 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.

Comment 10

[Zack Weinberg (:zwol)]

(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.

Comment 11

[Jeremy Lea]

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...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Zack Weinberg (:zwol)]

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.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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.

Comment 16

[Zack Weinberg (:zwol)]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 18

[Zack Weinberg (:zwol)]

Attachment: v3a: update for changes to nsRect/nsIntRect

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.

Comment 19

[Zack Weinberg (:zwol)]

Attachment: v3b: almost-working-but-not-quite templatified conversion functions

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?

Comment 20

[Zack Weinberg (:zwol)]

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!

Comment 21

[Zack Weinberg (:zwol)]

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.