Re: [Gnash-commit] gnash backend/render_handler.h backend/render_h...

[strk]

On Thu, Mar 01, 2007 at 10:33:22AM +0100, Udo Giacomozzi wrote:
> Hello strk,
> 
> Wednesday, February 28, 2007, 8:47:22 PM, you wrote:
> >> -     virtual void get_invalidated_bounds(rect* bounds, bool force) = 0;
> >> +     virtual void add_invalidated_bounds(InvalidatedRanges& ranges, bool 
> >> force) = 0;
> 
> s> As I was asking on IRC.. can we find a default implementation for this ?
> s> Like adding NOthing, or adding World ?
> s> make check doesn't work because this function is pure virtual.
> s> Not a big deal, I can make one, but I had the same problem this
> s> morning, with get_invalidated_bounds (that's why I asked).
> 
> If you insist. I made it intentionally pure virtual so that I find
> missing detail implementations at compile time, which is very helpful
> in our large class structure.

I won't insist.

> Most of the files in gnash are lowercase. Also class names are mostly
> lowercase (I was looking too much at Ranges2d class when choosing the
> name). BTW, we should agree to some standard naming convention
> (CamelCase vs. under_score etc.).

My own style so far has been to use CamelCase for new things.
I'm sure many will disagree...

> s> You can assert(temp.isNull()) right after constructor (Null is the 
> default).
> s>         Range2d(RangeKind kind=nullRange);
> 
> Looked cleaner.

These constructors are available:

        Range2d<int> nullIntRange2(nullRange); // get a Null
        Range2d<int> nullIntRange2(worldRange); // get a World
        Range2d<int> nullIntRange2(0, 0, 10, 10); // get a Finite

> That's why I didn't bother about _ranges.size(). I expect someone
> changing this to iterators. I failed on it since I'm not very
> experienced with them and my time is limited.

I'll be happy to do this, but will wait for a testcase first.

> >> +     /// returns true, when two ranges should be merged together
> >> +     inline bool snaptest(const RangeType& range1, const RangeType& 
> >> range2) {
> 
> s> How about renaming this to shouldMerge() or similar ?
> s> And, does the 'inline' part make any sense in a templated class ?
> 
> Why not? How are these two related?

I'd think templated functions would always be inlined, but I might be wrong.

> snaptest() is a method that's called very often and thus should be
> optimized.
> 
> 
> >> +             T xdist = 99999999;
> >> +             T ydist = 99999999;
> 
> s> No point in initializing these here, right ?
> s> Can initialize xdist = abs(xa1-xa2) below..
> 
> I'm avoiding abs() since it depends on variable type.

std::abs() should be an overloaded one, did you try ?

> s> I think we want contains(const Range2d<T>& ) too..
> 
> You are free to change this.

I'll do when approaching existing testcases.


--strk;