Enable clang-tidy bugprone-branch-clone (CleverRaven#49629)

* Enable bugprone-branch-clone This check looks for multiple branches of an else-if-chain or switch that are the same. This is often a sign of a copy/paste error. The cases that hit in these changes did not include any copy/paste errors, as far as I could see, but they did highlight quite a few cases where the code could be usefully refactored. I did that, and in other cases I just suppressed the error. Did fix one small bug related to furniture moving weariness, but that was just because I happened to be pointed at nearby code, not because it specifically related to the check. * Move utf32_to_utf8 to json.cpp When I deduplicated this function I previously kept the version in catacharset.cpp, but that breaks the link for the json formatter, so instead keep the version in json.cpp. Co-authored-by: actual-nh <[email protected]> Co-authored-by: Kevin Granade <[email protected]>
Maleclypse · Jul 12, 2021 · 159413d · 159413d
1 parent 4abcc6f
commit 159413d
Show file tree

Hide file tree

Showing 65 changed files with 365 additions and 581 deletions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -46,7 +46,6 @@ modernize-*,\
 performance-*,\
 readability-*,\
 -readability-braces-around-statements,\
--bugprone-branch-clone,\
 -bugprone-infinite-loop,\
 -bugprone-misplaced-widening-cast,\
 -bugprone-narrowing-conversions,\

diff --git a/src/action.cpp b/src/action.cpp
@@ -69,8 +69,8 @@ void parse_keymap( std::istream &keymap_txt, std::map<char, action_id> &kmap,
     while( !keymap_txt.eof() ) {
         std::string id;
         keymap_txt >> id;
-        if( id.empty() ) {
-            // Empty line, chomp it
+        if( id.empty() || id[0] == '#' ) {
+            // Empty line or comment, chomp it
             getline( keymap_txt, id );
         } else if( id == "unbind" ) {
             keymap_txt >> id;
@@ -79,7 +79,7 @@ void parse_keymap( std::istream &keymap_txt, std::map<char, action_id> &kmap,
                 unbound_keymap.insert( act );
             }
             break;
-        } else if( id[0] != '#' ) {
+        } else {
             const action_id act = look_up_action( id );
             if( act == ACTION_NULL ) {
                 debugmsg( "Warning!  keymap.txt contains an unknown action, \"%s\"\n"
@@ -101,9 +101,6 @@ void parse_keymap( std::istream &keymap_txt, std::map<char, action_id> &kmap,
                     }
                 }
             }
-        } else {
-            // Clear the whole line
-            getline( keymap_txt, id );
         }
     }
 }
@@ -659,7 +656,8 @@ bool can_examine_at( const tripoint &p )
 
     if( here.has_furn( p ) && xfurn_t.can_examine() ) {
         return true;
-    } else if( xter_t.can_examine() ) {
+    }
+    if( xter_t.can_examine() ) {
         return true;
     }
 

diff --git a/src/activity_handlers.cpp b/src/activity_handlers.cpp
@@ -892,10 +892,10 @@ static void butchery_drops_harvest( item *corpse_item, const mtype &mt, player &
         // QUICK BUTCHERY
         if( action == butcher_type::QUICK ) {
             if( entry.type == "flesh" ) {
-                roll = roll / 4;
-            } else if( entry.type == "bone" ) {
+                roll /= 4;
+            } else if( entry.type == "bone" ) { // NOLINT(bugprone-branch-clone)
                 roll /= 2;
-            } else if( corpse_item->get_mtype()->size >= creature_size::medium && ( entry.type == "skin" ) ) {
+            } else if( corpse_item->get_mtype()->size >= creature_size::medium && entry.type == "skin" ) {
                 roll /= 2;
             } else if( entry.type == "offal" ) {
                 roll /= 5;

diff --git a/src/activity_item_handling.cpp b/src/activity_item_handling.cpp
@@ -2336,10 +2336,14 @@ static std::unordered_set<tripoint> generic_multi_activity_locations( player &p,
         const tripoint set_pt = here.getlocal( *it2 );
         if( here.dangerous_field_at( set_pt ) ) {
             it2 = src_set.erase( it2 );
-            // remove tiles in darkness, if we aren't lit-up ourselves
-        } else if( !dark_capable && p.fine_detail_vision_mod( set_pt ) > 4.0 ) {
+            continue;
+        }
+        // remove tiles in darkness, if we aren't lit-up ourselves
+        if( !dark_capable && p.fine_detail_vision_mod( set_pt ) > 4.0 ) {
             it2 = src_set.erase( it2 );
-        } else if( act_id == ACT_MULTIPLE_FISH ) {
+            continue;
+        }
+        if( act_id == ACT_MULTIPLE_FISH ) {
             const ter_id terrain_id = here.ter( set_pt );
             if( !terrain_id.obj().has_flag( TFLAG_DEEP_WATER ) ) {
                 it2 = src_set.erase( it2 );

diff --git a/src/advanced_inv.cpp b/src/advanced_inv.cpp
@@ -845,7 +845,9 @@ bool advanced_inventory::move_all_items()
     if( dpane.get_area() == AIM_DRAGGED && sarea.pos == darea.pos &&
         spane.in_vehicle() == dpane.in_vehicle() ) {
         return false;
-    } else if( spane.get_area() == dpane.get_area() && spane.in_vehicle() == dpane.in_vehicle() ) {
+    }
+
+    if( spane.get_area() == dpane.get_area() && spane.in_vehicle() == dpane.in_vehicle() ) {
         return false;
     }
 

diff --git a/src/auto_pickup.cpp b/src/auto_pickup.cpp
@@ -672,8 +672,9 @@ void rule_list::create_rule( cache &map_items, const item &it )
     for( const rule &elem : *this ) {
         if( !elem.bActive ) {
             continue;
-        } else if( !check_special_rule( it.made_of(), elem.sRule ) &&
-                   !wildcard_match( to_match, elem.sRule ) ) {
+        }
+        if( !check_special_rule( it.made_of(), elem.sRule ) &&
+            !wildcard_match( to_match, elem.sRule ) ) {
             continue;
         }
 

diff --git a/src/cata_tiles.cpp b/src/cata_tiles.cpp
@@ -327,15 +327,10 @@ static void get_tile_information( const std::string &config_path, std::string &j
             std::string sOption;
             fin >> sOption;
 
-            if( sOption.empty() ) {
-                getline( fin, sOption );
-            } else if( sOption[0] == '#' ) {
-                // Skip comment
-                getline( fin, sOption );
-            } else if( sOption.find( "JSON" ) != std::string::npos ) {
+            if( string_starts_with( sOption, "JSON" ) ) {
                 fin >> json_path;
                 dbg( D_INFO ) << "JSON path set to [" << json_path << "].";
-            } else if( sOption.find( "TILESET" ) != std::string::npos ) {
+            } else if( string_starts_with( sOption, "TILESET" ) ) {
                 fin >> tileset_path;
                 dbg( D_INFO ) << "TILESET path set to [" << tileset_path << "].";
             } else {
@@ -2395,25 +2390,20 @@ bool cata_tiles::draw_terrain_below( const tripoint &p, const lit_level, int &,
     const furn_t &curr_furn = here.furn( pbelow ).obj();
     int part_below;
     int sizefactor = 2;
-    const vehicle *veh;
-    //        const vehicle *veh;
-    if( curr_furn.has_flag( TFLAG_SEEN_FROM_ABOVE ) ) {
+    if( curr_furn.has_flag( TFLAG_SEEN_FROM_ABOVE ) || curr_furn.movecost < 0 ) {
         tercol = curses_color_to_SDL( curr_furn.color() );
-    } else if( curr_furn.movecost < 0 ) {
-        tercol = curses_color_to_SDL( curr_furn.color() );
-    } else if( ( veh = here.veh_at_internal( pbelow, part_below ) ) != nullptr ) {
+    } else if( const vehicle *veh = here.veh_at_internal( pbelow, part_below ) ) {
         const int roof = veh->roof_at_part( part_below );
         const auto vpobst = vpart_position( const_cast<vehicle &>( *veh ),
                                             part_below ).obstacle_at_part();
         tercol = curses_color_to_SDL( ( roof >= 0 ||
                                         vpobst ) ? c_light_gray : c_magenta );
         sizefactor = ( roof >= 0 || vpobst ) ? 4 : 2;
-    } else if( curr_ter.has_flag( TFLAG_SEEN_FROM_ABOVE ) || curr_ter.movecost == 0 ) {
-        tercol = curses_color_to_SDL( curr_ter.color() );
-    } else if( !curr_ter.has_flag( TFLAG_NO_FLOOR ) ) {
-        sizefactor = 4;
+    } else if( curr_ter.has_flag( TFLAG_SEEN_FROM_ABOVE ) || curr_ter.has_flag( TFLAG_NO_FLOOR ) ||
+               curr_ter.movecost == 0 ) {
         tercol = curses_color_to_SDL( curr_ter.color() );
     } else {
+        sizefactor = 4;
         tercol = curses_color_to_SDL( curr_ter.color() );
     }
 
@@ -3546,8 +3536,6 @@ void cata_tiles::draw_custom_explosion_frame()
                 rotation = 3;
                 break;
             case N_NO_NEIGHBORS:
-                subtile = edge;
-                break;
             case N_WEST | N_EAST | N_NORTH | N_SOUTH:
                 // Needs some special tile
                 subtile = edge;

diff --git a/src/catacharset.cpp b/src/catacharset.cpp
@@ -98,46 +98,6 @@ uint32_t UTF8_getch( const char **src, int *srclen )
     return ch;
 }
 
-std::string utf32_to_utf8( uint32_t ch )
-{
-    char out[5];
-    char *buf = out;
-    static const unsigned char utf8FirstByte[7] = { 0x00, 0x00, 0xC0, 0xE0, 0xF0, 0xF8, 0xFC };
-    int utf8Bytes;
-    if( ch < 0x80 ) {
-        utf8Bytes = 1;
-    } else if( ch < 0x800 ) {
-        utf8Bytes = 2;
-    } else if( ch < 0x10000 ) {
-        utf8Bytes = 3;
-    } else if( ch <= 0x10FFFF ) {
-        utf8Bytes = 4;
-    } else {
-        utf8Bytes = 3;
-        ch = UNKNOWN_UNICODE;
-    }
-
-    buf += utf8Bytes;
-    switch( utf8Bytes ) {
-        case 4:
-            *--buf = ( ch | 0x80 ) & 0xBF;
-            ch >>= 6;
-        /* fallthrough */
-        case 3:
-            *--buf = ( ch | 0x80 ) & 0xBF;
-            ch >>= 6;
-        /* fallthrough */
-        case 2:
-            *--buf = ( ch | 0x80 ) & 0xBF;
-            ch >>= 6;
-        /* fallthrough */
-        case 1:
-            *--buf = ch | utf8FirstByte[utf8Bytes];
-    }
-    out[utf8Bytes] = '\0';
-    return out;
-}
-
 //Calculate width of a Unicode string
 //Latin characters have a width of 1
 //CJK characters have a width of 2, etc

diff --git a/src/cellular_automata.h b/src/cellular_automata.h
@@ -82,16 +82,13 @@ inline std::vector<std::vector<int>> generate_cellular_automaton( const int widt
                 // Count our neighbors.
                 const int neighbors = neighbor_count( current, width, height, point( i, j ) );
 
-                // Dead and > birth_limit neighbors, so become alive.
-                if( ( current[i][j] == 0 ) && ( neighbors > birth_limit ) ) {
+                // Dead and > birth_limit neighbors, become alive.
+                // Alive and > stasis_limit neighbors, stay alive.
+                const int to_live = current[i][j] ? stasis_limit : birth_limit;
+                if( neighbors > to_live ) {
                     next[i][j] = 1;
-                }
-                // Alive and > statis_limit neighbors, so stay alive.
-                else if( ( current[i][j] == 1 ) && ( neighbors > stasis_limit ) ) {
-                    next[i][j] = 1;
-                }
-                // Else, die.
-                else {
+                    // Else, die.
+                } else {
                     next[i][j] = 0;
                 }
             }

diff --git a/src/character.cpp b/src/character.cpp
@@ -724,9 +724,7 @@ double Character::aim_cap_from_volume( const item &gun ) const
 
     double aim_cap = std::min( 49.0, 49.0 - static_cast<float>( wielded_volume / 75_ml ) );
     // TODO: also scale with skill level.
-    if( gun_skill == skill_smg ) {
-        aim_cap = std::max( 12.0, aim_cap );
-    } else if( gun_skill == skill_shotgun ) {
+    if( gun_skill == skill_smg || gun_skill == skill_shotgun ) {
         aim_cap = std::max( 12.0, aim_cap );
     } else if( gun_skill == skill_pistol ) {
         aim_cap = std::max( 15.0, aim_cap * 1.25 );
@@ -2298,7 +2296,8 @@ bool Character::can_fuel_bionic_with( const item &it ) const
         for( const material_id &fuel : bid->fuel_opts ) {
             if( fuel == it.get_base_material().id ) {
                 return true;
-            } else if( it.type->magazine && fuel == item( it.ammo_current() ).get_base_material().id ) {
+            }
+            if( it.type->magazine && fuel == item( it.ammo_current() ).get_base_material().id ) {
                 return true;
             }
         }
@@ -2313,9 +2312,8 @@ std::vector<bionic_id> Character::get_bionic_fueled_with( const item &it ) const
 
     for( const bionic_id &bid : get_bionics() ) {
         for( const material_id &fuel : bid->fuel_opts ) {
-            if( fuel == it.get_base_material().id ) {
-                bionics.emplace_back( bid );
-            } else if( it.type->magazine && fuel == item( it.ammo_current() ).get_base_material().id ) {
+            if( fuel == it.get_base_material().id ||
+                ( it.type->magazine && fuel == item( it.ammo_current() ).get_base_material().id ) ) {
                 bionics.emplace_back( bid );
             }
         }
@@ -6997,7 +6995,7 @@ Character::comfort_response_t Character::base_comfort_value( const tripoint &p )
 
     // Some mutants have different comfort needs
     if( !plantsleep && !webforce ) {
-        if( in_shell ) {
+        if( in_shell ) { // NOLINT(bugprone-branch-clone)
             comfort += 1 + static_cast<int>( comfort_level::slightly_comfortable );
             // Note: shelled individuals can still use sleeping aids!
         } else if( vp ) {
@@ -7091,9 +7089,8 @@ Character::comfort_response_t Character::base_comfort_value( const tripoint &p )
                 }
             }
         }
-        if( fungaloid_cosplay && here.has_flag_ter_or_furn( "FUNGUS", pos() ) ) {
-            comfort += static_cast<int>( comfort_level::very_comfortable );
-        } else if( watersleep && here.has_flag_ter( "SWIMMABLE", pos() ) ) {
+        if( ( fungaloid_cosplay && here.has_flag_ter_or_furn( "FUNGUS", pos() ) ) ||
+            ( watersleep && here.has_flag_ter( "SWIMMABLE", pos() ) ) ) {
             comfort += static_cast<int>( comfort_level::very_comfortable );
         }
     } else if( plantsleep ) {
@@ -7227,11 +7224,9 @@ bodypart_id Character::body_window( const std::string &menu_header,
         const bool limb_is_broken = is_limb_broken( bp );
         const bool limb_is_mending = worn_with_flag( flag_SPLINT, bp );
 
-        if( show_all ) {
+        if( show_all || has_curable_effect ) { // NOLINT(bugprone-branch-clone)
             e.allowed = true;
-        } else if( has_curable_effect ) {
-            e.allowed = true;
-        } else if( limb_is_broken ) {
+        } else if( limb_is_broken ) { // NOLINT(bugprone-branch-clone)
             e.allowed = false;
         } else if( current_hp < maximal_hp && ( e.bonus != 0 || bandage_power > 0.0f  ||
                                                 disinfectant_power > 0.0f ) ) {
@@ -9044,9 +9039,6 @@ int Character::item_wear_cost( const item &it ) const
     double mv = item_handling_cost( it );
 
     switch( it.get_layer() ) {
-        case layer_level::PERSONAL:
-            break;
-
         case layer_level::UNDERWEAR:
             mv *= 1.5;
             break;
@@ -9063,9 +9055,8 @@ int Character::item_wear_cost( const item &it ) const
             mv /= 2.0;
             break;
 
+        case layer_level::PERSONAL:
         case layer_level::AURA:
-            break;
-
         default:
             break;
     }
@@ -11239,13 +11230,11 @@ int Character::temp_corrected_by_climate_control( int temperature ) const
             temperature = BODYTEMP_VERY_HOT;
         } else if( temperature > BODYTEMP_VERY_HOT ) {
             temperature = BODYTEMP_HOT;
-        } else if( temperature > BODYTEMP_HOT ) {
-            temperature = BODYTEMP_NORM;
         } else if( temperature < BODYTEMP_FREEZING ) {
             temperature = BODYTEMP_VERY_COLD;
         } else if( temperature < BODYTEMP_VERY_COLD ) {
             temperature = BODYTEMP_COLD;
-        } else if( temperature < BODYTEMP_COLD ) {
+        } else {
             temperature = BODYTEMP_NORM;
         }
     }
@@ -11448,11 +11437,14 @@ bool Character::has_fire( const int quantity ) const
 
     if( get_map().has_nearby_fire( pos() ) ) {
         return true;
-    } else if( has_item_with_flag( flag_FIRE ) ) {
+    }
+    if( has_item_with_flag( flag_FIRE ) ) {
         return true;
-    } else if( !find_firestarter_with_charges( quantity ).is_null() ) {
+    }
+    if( !find_firestarter_with_charges( quantity ).is_null() ) {
         return true;
-    } else if( is_npc() ) {
+    }
+    if( is_npc() ) {
         // HACK: A hack to make NPCs use their Molotovs
         return true;
     }
@@ -11497,21 +11489,22 @@ void Character::use_fire( const int quantity )
 
     if( get_map().has_nearby_fire( pos() ) ) {
         return;
-    } else if( has_item_with_flag( flag_FIRE ) ) {
+    }
+    if( has_item_with_flag( flag_FIRE ) ) {
+        return;
+    }
+
+    const item firestarter = find_firestarter_with_charges( quantity );
+    if( firestarter.is_null() ) {
+        return;
+    }
+    if( firestarter.type->has_flag( flag_USES_BIONIC_POWER ) ) {
+        mod_power_level( -quantity * 5_kJ );
+        return;
+    }
+    if( firestarter.typeId()->can_have_charges() ) {
+        use_charges( firestarter.typeId(), quantity );
         return;
-    } else {
-        const item firestarter = find_firestarter_with_charges( quantity );
-        if( firestarter.is_null() ) {
-            return;
-        }
-        if( firestarter.type->has_flag( flag_USES_BIONIC_POWER ) ) {
-            mod_power_level( -quantity * 5_kJ );
-            return;
-        }
-        if( firestarter.typeId()->can_have_charges() ) {
-            use_charges( firestarter.typeId(), quantity );
-            return;
-        }
     }
 }