From 9fad87b602a048335b475b7747df628352f2765b Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 9 May 2019 20:04:29 +0100 Subject: [PATCH 1/7] Enable some clang-tidy checks These ones seem to not be generating errors any more (probably because of #29937). clang-analyzer-cplusplus.NewDelete clang-diagnostic-defaulted-function-deleted clang-analyzer-optin.cplusplus.VirtualCall --- .clang-tidy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index 87b100911cc79..418a49abf0179 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,4 +1,4 @@ -Checks: 'clang-diagnostic-*,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-security.FloatLoopCounter,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.uninitialized.Assign,-clang-analyzer-cplusplus.NewDelete,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.NonNullParamChecker,-clang-analyzer-core.DivideZero,-clang-diagnostic-defaulted-function-deleted,-clang-analyzer-optin.cplusplus.VirtualCall' +Checks: 'clang-diagnostic-*,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-security.FloatLoopCounter,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.uninitialized.Assign,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.NonNullParamChecker,-clang-analyzer-core.DivideZero' WarningsAsErrors: '*' HeaderFilterRegex: '.*' FormatStyle: none From 4c73ceb8eaff023a0d1e0cbdd2671e077e016a0a Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 9 May 2019 20:55:40 +0100 Subject: [PATCH 2/7] Avoid division by zero in companion_rank clang-tidy is worried about this. I think it could only happen in practice if an NPC had some zero stats, but that might just be possible. --- src/mission_companion.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/mission_companion.cpp b/src/mission_companion.cpp index e9016a4625a5a..c351e623fa65b 100644 --- a/src/mission_companion.cpp +++ b/src/mission_companion.cpp @@ -1849,11 +1849,11 @@ std::vector talk_function::companion_rank( const std::vector } std::vector adjusted; - for( auto entry : raw ) { + for( const auto &entry : raw ) { comp_rank r; - r.combat = 100 * entry.combat / max_combat; - r.survival = 100 * entry.survival / max_survival; - r.industry = 100 * entry.industry / max_industry; + r.combat = max_combat ? 100 * entry.combat / max_combat : 0; + r.survival = max_survival ? 100 * entry.survival / max_survival : 0; + r.industry = max_industry ? 100 * entry.industry / max_industry : 0; adjusted.push_back( r ); } return adjusted; From 856d0c2cea4425d60ab50488c1631db4dec7101a Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 9 May 2019 21:50:14 +0100 Subject: [PATCH 3/7] Avoid division by zero in veh_interact This could only happen if every part on a vehicle had zero max HP, which is presumably impossible, but maybe could happen in a mod. Change is primarily to appease clang-tidy. --- src/veh_interact.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/veh_interact.cpp b/src/veh_interact.cpp index 5cedfc4f3dddf..2c14e9fb46b3a 100644 --- a/src/veh_interact.cpp +++ b/src/veh_interact.cpp @@ -2595,7 +2595,7 @@ void veh_interact::count_durability() return lhs + rhs.base.max_damage(); } ); - int pct = 100 * qty / total; + int pct = total ? 100 * qty / total : 0; if( pct < 5 ) { total_durability_text = _( "like new" ); From 55acf5a97ff96583c6ff5c2b7dc6822e9ffb628c Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 9 May 2019 21:52:06 +0100 Subject: [PATCH 4/7] Refator item::charges_per_volume Have a separate division-by-zero protection in each branch, and add debugmsg errors if zero-volume things happen in practice. --- src/item.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/item.cpp b/src/item.cpp index faeeacfc2fb57..89d5c3fa2bf73 100644 --- a/src/item.cpp +++ b/src/item.cpp @@ -597,12 +597,22 @@ item item::in_container( const itype_id &cont ) const long item::charges_per_volume( const units::volume &vol ) const { - if( type->volume == 0_ml ) { - return INFINITE_CHARGES; // TODO: items should not have 0 volume at all! + if( count_by_charges() ) { + if( type->volume == 0_ml ) { + debugmsg( "Item '%s' with zero volume", tname() ); + return INFINITE_CHARGES; + } + // Type cast to prevent integer overflow with large volume containers like the cargo + // dimension + return vol * static_cast( type->stack_size ) / type->volume; + } else { + units::volume my_volume = volume(); + if( my_volume == 0_ml ) { + debugmsg( "Item '%s' with zero volume", tname() ); + return INFINITE_CHARGES; + } + return vol / my_volume; } - // Type cast to prevent integer overflow with large volume containers like the cargo dimension - return count_by_charges() ? vol * static_cast( type->stack_size ) / type->volume - : vol / volume(); } bool item::stacks_with( const item &rhs, bool check_components ) const From 85062aee8f6c27779150d15c3c43b4d1b0cb62e6 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 9 May 2019 21:57:48 +0100 Subject: [PATCH 5/7] Check for division by zero in vehicle fuel calc Unclear whether this can happen in practice. The logic is rather complex. Adding a debugmsg as a better option than a crash on divide by zero. I'm informed that this code should change in the long term anyway. --- src/vehicle.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/vehicle.cpp b/src/vehicle.cpp index 726fcde505484..e5ad91563ef53 100644 --- a/src/vehicle.cpp +++ b/src/vehicle.cpp @@ -2972,7 +2972,15 @@ int vehicle::consumption_per_hour( const itype_id &ftype, int fuel_rate_w ) cons // add 3600 seconds worth of fuel consumption for the engine // HACK - engines consume 1 second worth of fuel per turn, even though a turn is 6 seconds if( vslowdown > 0 ) { - amount_pct += 600 * vslowdown / acceleration( true, target_v ); + int accel = acceleration( true, target_v ); + if( accel == 0 ) { + // FIXME: Long-term plan is to change the fuel consumption + // computation entirely; for now just warn if this would + // otherwise have been division-by-zero + debugmsg( "Vehicle unexpectedly has zero acceleration" ); + } else { + amount_pct += 600 * vslowdown / accel; + } } } int energy_j_per_mL = fuel.fuel_energy() * 1000; From f218733466471a34bc7a5b24e1353f674a15a3b0 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 9 May 2019 21:59:56 +0100 Subject: [PATCH 6/7] Enable clang-analyzer-core.DivideZero check Having worked around all the current hits for this check, enable it in .clang-tidy. --- .clang-tidy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index 418a49abf0179..867b80632913f 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,4 +1,4 @@ -Checks: 'clang-diagnostic-*,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-security.FloatLoopCounter,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.uninitialized.Assign,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.NonNullParamChecker,-clang-analyzer-core.DivideZero' +Checks: 'clang-diagnostic-*,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-security.FloatLoopCounter,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.uninitialized.Assign,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.NonNullParamChecker' WarningsAsErrors: '*' HeaderFilterRegex: '.*' FormatStyle: none From aa606c749e199a32ec01b87933c7920cc871c3b8 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Fri, 10 May 2019 16:06:57 +0100 Subject: [PATCH 7/7] Add some checks back in These manifested on Travis; I guess it's running a slightly different version of clang-tidy. --- .clang-tidy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index 867b80632913f..fac1beb1f2f82 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,4 +1,4 @@ -Checks: 'clang-diagnostic-*,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-security.FloatLoopCounter,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.uninitialized.Assign,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.NonNullParamChecker' +Checks: 'clang-diagnostic-*,-clang-analyzer-deadcode.DeadStores,-clang-analyzer-security.FloatLoopCounter,-clang-analyzer-core.UndefinedBinaryOperatorResult,-clang-analyzer-core.uninitialized.Assign,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-core.CallAndMessage,-clang-analyzer-core.NonNullParamChecker,-clang-analyzer-optin.cplusplus.VirtualCall,-clang-analyzer-core.NullDereference' WarningsAsErrors: '*' HeaderFilterRegex: '.*' FormatStyle: none