Review: Needs Fixing
Diff comments:
>
> === modified file 'src/base/md5.h'
> --- src/base/md5.h 2017-01-25 18:55:59 +0000
> +++ src/base/md5.h 2017-06-28 08:25:35 +0000
> @@ -74,7 +74,7 @@
> */
> template <typename Base> class MD5Checksum : public Base {
> public:
> - MD5Checksum() {
> + MD5Checksum() : Base() {
unnecessary, the default constructor will always be called.
> Reset();
> }
> explicit MD5Checksum(const MD5Checksum& other)
>
> === modified file 'src/economy/shippingitem.h'
> --- src/economy/shippingitem.h 2017-06-24 08:47:46 +0000
> +++ src/economy/shippingitem.h 2017-06-28 08:25:35 +0000
> @@ -59,6 +59,8 @@
> void remove(EditorGameBase&);
>
> struct Loader {
> + Loader () : serial_(0U) {
c++11 allows to initial PODs inline. I think that is preferable to adding
constructors, i.e. just write
int serial_ = 0;
below.
> + }
> void load(FileRead& fr);
> ShippingItem get(MapObjectLoader& mol);
>
>
> === modified file 'src/game_io/game_preload_packet.cc'
> --- src/game_io/game_preload_packet.cc 2017-01-25 18:55:59 +0000
> +++ src/game_io/game_preload_packet.cc 2017-06-28 08:25:35 +0000
> @@ -46,6 +46,9 @@
> constexpr uint16_t kCurrentPacketVersion = 6;
> constexpr const char* kMinimapFilename = "minimap.png";
>
> +GamePreloadPacket::GamePreloadPacket() : minimap_path_(""), mapname_(""),
> background_(""), win_condition_(""), gametime_(0),
> player_nr_(Widelands::neutral()), version_(""), savetimestamp_(0),
> gametype_(GameController::GameType::kUndefined) {
unnecessary to initially std::string, it will always initialized to be empty.
inline initialize the PODs in the header, should also work for the enum.
> +}
> +
> std::string GamePreloadPacket::get_localized_win_condition() const {
> i18n::Textdomain td("win_conditions");
> return _(win_condition_);
>
> === modified file 'src/graphic/gl/fields_to_draw.h'
> --- src/graphic/gl/fields_to_draw.h 2017-05-13 18:48:26 +0000
> +++ src/graphic/gl/fields_to_draw.h 2017-06-28 08:25:35 +0000
> @@ -78,6 +78,7 @@
> };
>
> FieldsToDraw() {
> + reset(0, 0, 0, 0);
This should always be reset before used anyways. How about adding a comment
that this is to appease cppcheck?
> }
>
> // Resize this fields to draw for reuse.
>
> === modified file 'src/graphic/graphic.cc'
> --- src/graphic/graphic.cc 2017-05-31 21:27:07 +0000
> +++ src/graphic/graphic.cc 2017-06-28 08:25:35 +0000
> @@ -59,7 +59,7 @@
>
> } // namespace
>
> -Graphic::Graphic() : image_cache_(new ImageCache()), animation_manager_(new
> AnimationManager()) {
> +Graphic::Graphic() : window_mode_width_(0), window_mode_height_(0),
> sdl_window_(nullptr), max_texture_size_(kMinimumSizeForTextures),
> image_cache_(new ImageCache()), animation_manager_(new AnimationManager()) {
inline initialize POD
> }
>
> /**
>
> === modified file 'src/graphic/render_queue.h'
> --- src/graphic/render_queue.h 2017-01-25 18:55:59 +0000
> +++ src/graphic/render_queue.h 2017-06-28 08:25:35 +0000
> @@ -113,7 +113,7 @@
> };
>
> struct TerrainArguments {
> - TerrainArguments() {
> + TerrainArguments() : gametime(0), renderbuffer_width(0),
> renderbuffer_height(0), fields_to_draw(nullptr), scale(1.f) {
here too and below. will not repeat every time.
> }
>
> int gametime;
>
> === modified file 'src/logic/field.cc'
> --- src/logic/field.cc 2017-01-25 18:55:59 +0000
> +++ src/logic/field.cc 2017-06-28 08:25:35 +0000
> @@ -23,6 +23,9 @@
>
> namespace Widelands {
>
> +Field::Field() : bobs(nullptr), immovable(nullptr) , caps(7), roads(6),
> height(0), brightness(0), owner_info_and_selections(Widelands::neutral()),
> resources(INVALID_INDEX), initial_res_amount(0), res_amount(0),
> terrains{INVALID_INDEX, INVALID_INDEX} {
I guess this was intentionally left uninitialized because we make many of these
fields and will never use the default constructed value anyways. I doubt that
we care about that performance anymore, since it will only be done once when
the game is started. I suggest inline initializing this as much as possible too.
> (Seems like someone was trying to save some memory there. Not sure whether
> this try is successful.)
There is a static_assert to check that it is: static_assert(sizeof(Field) ==
sizeof(void*) * 2 + 10, "Field is not tightly packed."); so on 64bit
architecture, Field is 8*2+10 = 26 bytes. On a 512x512 map this amounts to
~6MB. I guess the bit fiddling reaches back to a time where this was a lot.
Player::Field does not use this bid fiddling, and IMHO every Player has a full
copy of all fields, so for 8 players we are ~50MB only for the maps.
After this analysis I agree that the bid fiddling probably does not make a lot
of sense in this day and age anymore.
> +}
> +
> /**
> * Set the field's brightness based upon the slopes.
> * Slopes are calulated as this field's height - neighbour's height.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
