[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands has been updated. Status: Needs review => Merged For more details, see: 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Review: Approve lgtm. @bunnybot merge -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Review: Resubmit I have now gone through the code base and removed all empty string initializations. I also found a few more uninitialized variables in MapTriangleRegion. If you agree with these changes, the branch will be ready. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands has been updated. Commit Message changed to: Initialize a bunch of uninitialized member variables, adding constructors where necessary. Turned some enums into enum classes. Removed empty string initializations. For more details, see: 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
I am currently running the report, which will probably take all day. I'll then run it again without the string init to see if it makes a difference. I'll then revert any changes in this branch that won't fix the report. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
> Well, it's just the same as initializing numbers to 0 - cppcheck is > complaining, so I'm trying to get rid of that noise in the report. That would be wrong though - plain old datatypes (POD) are not initialized in cpp, so numbers are indeed a random value if not properly initialized. std::string is a class, not a POD though so its constructor is always run. If cppcheck complains about this it would be wrong. I wonder if it is worthwhile to clutter the code with unnecessary (and for std::string even slightly decremental) changes to appease the tool. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Well, it's just the same as initializing numbers to 0 - cppcheck is complaining, so I'm trying to get rid of that noise in the report. The actual error message is: src/game_io/game_preload_packet.h:38: (style) The struct 'GamePreloadPacket' does not have a constructor although it has private member variables. Member variables of builtin types are left uninitialized when the class is instantiated. That may cause bugs or undefined behavior. I then proceeded to create a constructor, and you then advised to directly initialize the variables. The only way to test if this will shut up cppcheck is to run the full report again. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Review: Approve
3 nits about initializing std::string = "", otherwise lgtm. I think this can go
in once you have a look at the nits.
Diff comments:
>
> === modified file 'src/game_io/game_preload_packet.h'
> --- src/game_io/game_preload_packet.h 2017-01-25 18:55:59 +
> +++ src/game_io/game_preload_packet.h 2017-08-10 07:51:51 +
> @@ -74,16 +74,17 @@
> }
>
> private:
> - std::string minimap_path_;
> - std::string mapname_;
> - std::string background_;
> - std::string win_condition_;
> - uint32_t gametime_;
> - uint8_t player_nr_; // The local player idx
> - uint8_t number_of_players_;
> - std::string version_;
> - time_t savetimestamp_;
> - GameController::GameType gametype_;
> + // Initializing everything to make cppcheck happy.
> + std::string minimap_path_ = "";
Initializing a std::string is unnecessary, it is always the empty string. Does
cppcheck really complain about this?
> + std::string mapname_ = "";
> + std::string background_ = "";
> + std::string win_condition_ = "";
> + uint32_t gametime_ = 0U;
> + uint8_t player_nr_ = 0U; // The local player idx
> + uint8_t number_of_players_ = 0U;
> + std::string version_ = "";
> + time_t savetimestamp_ = 0;
> + GameController::GameType gametype_ =
> GameController::GameType::kUndefined;
> };
> }
>
>
> === modified file 'src/map_io/map_elemental_packet.cc'
> --- src/map_io/map_elemental_packet.cc2017-01-25 18:55:59 +
> +++ src/map_io/map_elemental_packet.cc2017-08-10 07:51:51 +
> @@ -33,6 +33,9 @@
> constexpr int32_t kEightPlayersPacketVersion = 1;
> constexpr int32_t kSixteenPlayersPacketVersion = 2;
>
> +MapElementalPacket::MapElementalPacket() : old_world_name_(""), version_(0) {
no need for std::string initialization?
> +}
> +
> void MapElementalPacket::pre_read(FileSystem& fs, Map* map) {
> Profile prof;
> prof.read("elemental", nullptr, fs);
>
> === modified file 'src/network/gameclient.cc'
> --- src/network/gameclient.cc 2017-07-05 19:21:57 +
> +++ src/network/gameclient.cc 2017-08-10 07:51:51 +
> @@ -820,8 +820,7 @@
> }
>
> case NETCMD_CHAT: {
> - ChatMessage c;
> - c.time = time(nullptr);
> + ChatMessage c("");
Here too?
> c.playern = packet.signed_16();
> c.sender = packet.string();
> c.msg = packet.string();
--
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 : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Continuous integration builds have changed state: Travis build 2518. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/262995097. Appveyor build 2342. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_986611_cppcheck_uninitialized_variables-2342. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Continuous integration builds have changed state: Travis build 2517. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/262810177. Appveyor build 2341. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_986611_cppcheck_uninitialized_variables-2341. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Review: Resubmit Ready for the next round. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Continuous integration builds have changed state: Travis build 2391. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/247860909. Appveyor build 2219. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_986611_cppcheck_uninitialized_variables-2219. -- 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 : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Review: Needs Fixing
Diff comments:
>
> === modified file 'src/base/md5.h'
> --- src/base/md5.h2017-01-25 18:55:59 +
> +++ src/base/md5.h2017-06-28 08:25:35 +
> @@ -74,7 +74,7 @@
> */
> template 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.h2017-06-24 08:47:46 +
> +++ src/economy/shippingitem.h2017-06-28 08:25:35 +
> @@ -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.cc2017-01-25 18:55:59 +
> +++ src/game_io/game_preload_packet.cc2017-06-28 08:25:35 +
> @@ -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 +
> +++ src/graphic/gl/fields_to_draw.h 2017-06-28 08:25:35 +
> @@ -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.cc2017-05-31 21:27:07 +
> +++ src/graphic/graphic.cc2017-06-28 08:25:35 +
> @@ -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.h2017-01-25 18:55:59 +
> +++ src/graphic/render_queue.h2017-06-28 08:25:35 +
> @@ -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.cc2017-01-25 18:55:59 +
> +++ src/logic/field.cc2017-06-28 08:25:35 +
> @@ -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.
> +}
>
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
I only checked a couple of the build logs, but the Travis errors seems to be due to packages failing to install. Is there an easy way to retrigger builds? I haven't looked into the details of the changes, but it is great to see the amount of issues reduced. This seems to be the last branch in review now that should have a large impact on the cppcheck report. I'll keep an eye out for when it is merged, and upload a new report :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Thanks for your efforts on cleaning up the code!
Most of this branch looks good to me, although I have a a few comments. The
first two points should probably be fixed, the other ones are not so important.
Note that I haven't tried compiling or testing, I only looked at the code.
Diff comments:
>
> === modified file 'src/graphic/gl/fields_to_draw.h'
> --- src/graphic/gl/fields_to_draw.h 2017-05-13 18:48:26 +
> +++ src/graphic/gl/fields_to_draw.h 2017-06-28 08:25:35 +
> @@ -78,6 +78,7 @@
> };
>
> FieldsToDraw() {
> + reset(0, 0, 0, 0);
With this we end up with w_=h_=1 and field_.size() of 1. I think the values of
w_ and h_ are fine (checks are done for <w_) but a size() of 0 would be
preferable. Haven't checked whether anyone cares about the size, though.
> }
>
> // Resize this fields to draw for reuse.
>
> === modified file 'src/logic/field.cc'
> --- src/logic/field.cc2017-01-25 18:55:59 +
> +++ src/logic/field.cc2017-06-28 08:25:35 +
> @@ -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} {
Where are the values 7 and 6 are coming from? As far as I know, the ": 7" below
is not an initialization but a hint to the compiler, how many bits this
variable uses.
(Seems like someone was trying to save some memory there. Not sure whether this
try is successful.)
> +}
> +
> /**
> * Set the field's brightness based upon the slopes.
> * Slopes are calulated as this field's height - neighbour's height.
>
> === modified file 'src/logic/map_objects/tribes/militarysite.h'
> --- src/logic/map_objects/tribes/militarysite.h 2017-06-25 19:12:30
> +
> +++ src/logic/map_objects/tribes/militarysite.h 2017-06-28 08:25:35
> +
> @@ -34,6 +34,13 @@
> class Soldier;
> class World;
>
> +// I assume elsewhere, that enum SoldierPreference fits to uint8_t.
> +enum class SoldierPreference : uint8_t {
> + kNotSet,
I think we only need kNotSet for a few seconds when loading a savegame. Maybe
just drop it?
> + kRookies,
> + kHeroes,
> +};
> +
> class MilitarySiteDescr : public BuildingDescr {
> public:
> MilitarySiteDescr(const std::string& init_descname,
>
> === modified file 'src/logic/map_objects/world/map_gen.cc'
> --- src/logic/map_objects/world/map_gen.cc2017-01-25 18:55:59 +
> +++ src/logic/map_objects/world/map_gen.cc2017-06-28 08:25:35 +
> @@ -84,7 +84,7 @@
>
> MapGenAreaInfo::MapGenAreaInfo(const LuaTable& table,
> const World& world,
> - MapGenAreaType const areaType) {
> + MapGenAreaType const area_type) {
Why not also rename the parameter in the other methods?
> weight_ = get_positive_int(table, "weight");
>
> const auto read_terrains = [this, , ](
>
> === modified file 'src/wui/debugconsole.cc'
> --- src/wui/debugconsole.cc 2017-01-25 18:55:59 +
> +++ src/wui/debugconsole.cc 2017-06-28 08:25:35 +
> @@ -90,6 +91,7 @@
> ChatMessage cm;
>
> cm.time = time(nullptr);
> + cm.playern = Widelands::neutral();
Maybe create a constructor for chat messages which only takes a message?
> cm.msg = msg;
> messages.push_back(cm);
>
--
https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into
lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Continuous integration builds have changed state: Travis build 2391. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/247860909. Appveyor build 2219. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_986611_cppcheck_uninitialized_variables-2219. -- https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
Continuous integration builds have changed state: Travis build 2356. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/246523458. Appveyor build 2184. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_986611_cppcheck_uninitialized_variables-2184. -- https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into
lp:widelands.
Commit message:
Initialize a bunch of uninitialized member variables, adding constructors where
necessary. Turned some enums into enum classes.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #986611 in widelands: "Issues reported by cppcheck"
https://bugs.launchpad.net/widelands/+bug/986611
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256
I'm on a code cleanup rampage today... this one needs a bit of looking at to
make sure that I initialized everything correctly.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into
lp:widelands.
=== modified file 'src/ai/defaultai_warfare.cc'
--- src/ai/defaultai_warfare.cc 2017-06-23 16:16:28 +
+++ src/ai/defaultai_warfare.cc 2017-06-24 10:45:33 +
@@ -612,9 +612,9 @@
if (ms->economy().warehouses().size()) {
uint32_t const j = ms->soldier_capacity();
- if (MilitarySite::kPrefersRookies != ms->get_soldier_preference()) {
+ if (SoldierPreference::kRookies != ms->get_soldier_preference()) {
game().send_player_militarysite_set_soldier_preference(
- *ms, MilitarySite::kPrefersRookies);
+ *ms, SoldierPreference::kRookies);
} else if (j > 1) {
game().send_player_change_soldier_capacity(*ms, (j > 2) ? -2 : -1);
}
@@ -672,9 +672,9 @@
changed = true;
// and also set preference to Heroes
- if (MilitarySite::kPrefersHeroes != ms->get_soldier_preference()) {
+ if (SoldierPreference::kHeroes != ms->get_soldier_preference()) {
game().send_player_militarysite_set_soldier_preference(
- *ms, MilitarySite::kPrefersHeroes);
+ *ms, SoldierPreference::kHeroes);
changed = true;
}
@@ -683,9 +683,9 @@
} else { // otherwise decrease soldiers
uint32_t const j = ms->soldier_capacity();
- if (MilitarySite::kPrefersRookies != ms->get_soldier_preference()) {
+ if (SoldierPreference::kRookies != ms->get_soldier_preference()) {
game().send_player_militarysite_set_soldier_preference(
- *ms, MilitarySite::kPrefersRookies);
+ *ms, SoldierPreference::kRookies);
} else if (j > 1) {
game().send_player_change_soldier_capacity(*ms, (j > 2) ? -2 : -1);
}
=== modified file 'src/base/md5.h'
--- src/base/md5.h 2017-01-25 18:55:59 +
+++ src/base/md5.h 2017-06-24 10:45:33 +
@@ -74,7 +74,7 @@
*/
template class MD5Checksum : public Base {
public:
- MD5Checksum() {
+ MD5Checksum() : Base() {
Reset();
}
explicit MD5Checksum(const MD5Checksum& other)
=== modified file 'src/economy/shippingitem.h'
--- src/economy/shippingitem.h 2017-01-25 18:55:59 +
+++ src/economy/shippingitem.h 2017-06-24 10:45:33 +
@@ -59,6 +59,8 @@
void remove(EditorGameBase&);
struct Loader {
+ Loader () : serial_(0U) {
+ }
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 +
+++ src/game_io/game_preload_packet.cc 2017-06-24 10:45:33 +
@@ -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) {
+}
+
std::string GamePreloadPacket::get_localized_win_condition() const {
i18n::Textdomain td("win_conditions");
return _(win_condition_);
=== modified file 'src/game_io/game_preload_packet.h'
--- src/game_io/game_preload_packet.h 2017-01-25 18:55:59 +
+++ src/game_io/game_preload_packet.h 2017-06-24 10:45:33 +
@@ -36,6 +36,8 @@
*/
struct GamePreloadPacket : public GameDataPacket {
+ GamePreloadPacket();
+
void read(FileSystem&, Game&, MapObjectLoader* = nullptr) override;
void write(FileSystem&, Game&, MapObjectSaver* = nullptr) override;
=== modified file 'src/graphic/gl/fields_to_draw.h'
--- src/graphic/gl/fields_to_draw.h 2017-05-13 18:48:26 +
+++ src/graphic/gl/fields_to_draw.h 2017-06-24 10:45:33 +
@@ -78,6 +78,7 @@
};
FieldsToDraw() {
+ reset(0, 0, 0, 0);
}
// Resize this fields to draw for reuse.
=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc 2017-05-31 21:27:07 +
+++ src/graphic/graphic.cc 2017-06-24 10:45:33 +
@@ -59,7 +59,7 @@
} // namespace
-Graphic::Graphic() : image_cache_(new ImageCache()), animation_manager_(new AnimationManager()) {
+Graphic::G
