widelands-dev team mailing list archive

[Nicolai Hähnle <prefect_@xxxxxxx>]

Nice work! The InMemoryImage is still a bit of a hack, but all the rest is now much cleaner than it used to be. I like the overall architecture a lot. Some comments on the details:

1. You use non-Doxygen formatted comments a lot in places where it would be nicer to have Doxygen comments. (The Doxygen build is overall not in the best shape, but still...)

2. constants.h: In C++, one is supposed to include cstdint etc. instead of stdint.h etc. (unless perhaps there are some C++ compilers where those files do not exist?)

3. editor/editorinteractive.cc: around line 155, the c_str() in immname.c_str() is not necessary

4. AnimationGfx: Why is the image cache stored again in instance of this class instead of just accessing it via g_gr like everywhere else?

5. graphic/graphic.h: The comment above the declaration of Graphic is outdated, maybe it's time to change that ;)

6. Compiling with g++, there are many warnings of the type "declaration of XXX shadows a member of 'this'". I guess you're using clang on Mac OSX which doesn't report such warnings? Here are some examples:
[  1%] Building CXX object src/CMakeFiles/widelands_all.dir/graphic/image_transformations.cc.o
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In constructor ‘{anonymous}::TransformedImage::TransformedImage(const string&, const Image&, SurfaceCache*)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:287:91: warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In constructor ‘{anonymous}::ResizedImage::ResizedImage(const string&, const Image&, SurfaceCache*, uint16_t, uint16_t)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:319:3: warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In constructor ‘{anonymous}::GrayedOutImage::GrayedOutImage(const string&, const Image&, SurfaceCache*)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:341:89: warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In constructor ‘{anonymous}::ChangeLuminosityImage::ChangeLuminosityImage(const string&, const Image&, SurfaceCache*, float, bool)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:358:3: warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In constructor ‘{anonymous}::PlayerColoredImage::PlayerColoredImage(const string&, const Image&, SurfaceCache*, const RGBColor&, const Image&)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:381:3: warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
[  3%] Building CXX object src/CMakeFiles/widelands_all.dir/graphic/render/gl_surface.cc.o
/home/nha/dev/widelands/repo/autocache/src/graphic/render/gl_surface.cc: In member function ‘virtual void GLSurface::draw_line(int32_t, int32_t, int32_t, int32_t, const RGBColor&, uint8_t)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/render/gl_surface.cc:157:87: warning: declaration of ‘width’ shadows a member of 'this' [-Wshadow]

-- 
https://code.launchpad.net/~sirver/widelands/autocache/+merge/147559
Your team Widelands Developers is requested to review the proposed merge of lp:~sirver/widelands/autocache into lp:widelands.