Code review comment for lp:~nha/widelands/opengl

Revision history for this message
Nicolai Hähnle (nha) wrote :

Nice, and thank you for merging those two branches! I have to run for now, so I would suggest you just do the merge into trunk whenever you're happy with it. Just some quick replies:

1. get_dither_edge_texture(): Since this is such a special case for now, I would just go directly to the SurfaceCache, but I don't have strong feelings about it. I haven't looked at your merge yet.

2. const-modifiers: Sigra was always very vocal about putting the const at the end, because that is more consistent with const-ness when you have pointers-to-pointers, and that just rubbed off a little bit onto me, but I have no strong feelings either way.

3. GameRenderer: Yes, should be boost::noncopyable, forgot about that.

4. const local variables: Compilers should be grown-up enough to be able to do optimizations themselves, but it does make sense to avoid future refactoring bugs.

5. Yes, those dynamic_casts could be static_casts, but it's not a performance critical path. In the spirit of defensive programming, one could leave it as is.

6. compile-time asserts: You're absolutely right about that.

« Back to merge proposal