The International Simutrans Forum

 

Author Topic: [r8797] initialization of convoi_t::recalc_speed_limit  (Read 1153 times)

0 Members and 1 Guest are viewing this topic.

Offline THLeaderH jp

  • Coder/patcher
  • Devotee
  • *
  • Posts: 335
  • Languages: JP,EN
[r8797] initialization of convoi_t::recalc_speed_limit
« on: August 05, 2019, 02:41:56 PM »
I noticed that convoi_t::recalc_speed_limit is not initialized when a convoi_t object is created. In general, not initializing variables can cause unexpected behaviors and network game de-sync.

The solution is inserting the following code into L166 and L2002 of simconvoi.cc.
Code: [Select]
recalc_speed_limit = true;

Thank you for your attention.

Offline DrSuperGood

  • Dev Team
  • Devotee
  • *
  • Posts: 2727
  • Languages: EN
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #1 on: August 05, 2019, 05:16:22 PM »
I noticed that convoi_t::recalc_speed_limit is not initialized when a convoi_t object is created. In general, not initializing variables can cause unexpected behaviors and network game de-sync.
I am a strong supporter of state initialization. However in theory it only is an issue if the variable is used before being set. If it never is used before being set then in theory there is no need to set it during object creation.

However one might as well set it for better code quality.
« Last Edit: August 06, 2019, 06:26:52 AM by DrSuperGood »

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #2 on: August 06, 2019, 05:29:23 AM »
This isn't technically about uninitialized variables. Member variables in C++ are always default initialized if no explicit initialization takes place in the constructors initializer list. This is about a member variable that isn't (re)set along with a bunch of others in certain conditions.

Looking through the code, recalc_speed_limit looks like a way to signal that part of the state is stale, rather than proper state itself. It is not saved or sent over the network in any way. Setting recalc_data to true signals exactly the same state recalculation, so setting both to true is superfluous. As far as I can see, setting either one to true has exactly the same effect. One might just as well merge them.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9569
  • Languages: De,EN,JP
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #3 on: August 06, 2019, 01:00:46 PM »
recalc_data also calculates the weight, which iterates over all ware_t it has loaded. So there is a reason to have both. But initialising cannot hurt.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #4 on: August 06, 2019, 06:41:44 PM »
The searches I did didn't reveal any if (recalc_data). All I found was if (recalc_data || recalc_speed_limit).

But initialising cannot hurt.
It was always initialized.

Offline THLeaderH jp

  • Coder/patcher
  • Devotee
  • *
  • Posts: 335
  • Languages: JP,EN
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #5 on: August 07, 2019, 02:38:38 AM »
I posted this because Undefined Behavior Sanitizer (UBSan) reported me that recalc_speed_limit was read and it had illegal value as a bool variable. Anyway, thank you for incorporating.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9569
  • Languages: De,EN,JP
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #6 on: August 08, 2019, 02:15:36 PM »
The recalculation of the weight is only done, when recalc_data is set. Two line below the above mentioned if

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: [r8797] initialization of convoi_t::recalc_speed_limit
« Reply #7 on: August 22, 2019, 11:46:46 AM »
Just for the record: I got my languages messed up apparently. C++ does not appear to initialize primitive member variables, just whatever the other kind is. Java initializes both primitives and its other kind.