Enable clang-tidy check bugprone-unused-return-value (#48546)

* Fix bugprone-unused-return-value

Biggest change here was to mapbuffer, which now stores submaps via
unique_ptr rather than raw pointers, simplifying the destructor.

Unfortunately, we have to retain the strange pointer-based API for
mapbuffer, because the submap ownership logic in map is so convoluted.
That can be fixed another day.

Also, renamed mapbuffer::reset() to clear(), for consistency.

In trait_group, removed a couple of calls to reset() that this warning
was highlighting.  They didn't do anything anyway.

* Enable clang-tidy bugprone-unused-return-value

.clang-tidy

@@ -23,7 +23,6 @@ performance-*,\
 readability-*,\
 -bugprone-misplaced-widening-cast,\
 -bugprone-narrowing-conversions,\
--bugprone-unused-return-value,\
 -cert-flp30-c,\
 -misc-non-private-member-variables-in-classes,\
 -modernize-avoid-c-arrays,\

src/game.cpp

@@ -416,7 +416,7 @@ bool game::check_mod_data( const std::vector<mod_id> &opts, loading_ui &ui )
         std::string world_name = world_generator->active_world->world_name;
         world_generator->delete_world( world_name, true );
 
-        MAPBUFFER.reset();
+        MAPBUFFER.clear();
         overmap_buffer.clear();
     }
 
@@ -460,7 +460,7 @@ bool game::check_mod_data( const std::vector<mod_id> &opts, loading_ui &ui )
         std::string world_name = world_generator->active_world->world_name;
         world_generator->delete_world( world_name, true );
 
-        MAPBUFFER.reset();
+        MAPBUFFER.clear();
         overmap_buffer.clear();
     }
     return true;
@@ -1285,7 +1285,7 @@ bool game::cleanup_at_end()
     sfx::fade_audio_group( sfx::group::context_themes, 300 );
     sfx::fade_audio_group( sfx::group::fatigue, 300 );
 
-    MAPBUFFER.reset();
+    MAPBUFFER.clear();
     overmap_buffer.clear();
 
 #if defined(__ANDROID__)
@@ -12254,7 +12254,7 @@ void game::quickload()
 
     if( active_world->save_exists( save_t::from_player_name( u.name ) ) ) {
         if( moves_since_last_save != 0 ) { // See if we need to reload anything
-            MAPBUFFER.reset();
+            MAPBUFFER.clear();
             overmap_buffer.clear();
             try {
                 setup();

src/main_menu.cpp

@@ -882,7 +882,7 @@ bool main_menu::new_character_tab()
                     }
                     if( !player_character.create( play_type ) ) {
                         load_char_templates();
-                        MAPBUFFER.reset();
+                        MAPBUFFER.clear();
                         overmap_buffer.clear();
                         continue;
                     }
@@ -954,7 +954,7 @@ bool main_menu::new_character_tab()
                 }
                 if( !player_character.create( character_type::TEMPLATE, templates[sel3] ) ) {
                     load_char_templates();
-                    MAPBUFFER.reset();
+                    MAPBUFFER.clear();
                     overmap_buffer.clear();
                     continue;
                 }
@@ -1247,7 +1247,7 @@ void main_menu::world_tab()
                 player_character.save_template( player_character.name, points );
 
                 player_character = avatar();
-                MAPBUFFER.reset();
+                MAPBUFFER.clear();
                 overmap_buffer.clear();
 
                 load_char_templates();
@@ -1323,7 +1323,7 @@ void main_menu::world_tab()
                         world_generator->delete_world( all_worldnames[sel2 - 1], do_delete );
 
                         savegames.clear();
-                        MAPBUFFER.reset();
+                        MAPBUFFER.clear();
                         overmap_buffer.clear();
 
                         if( do_delete ) {

src/mapbuffer.cpp

@@ -43,36 +43,32 @@ static std::string find_dirname( const tripoint &om_addr )
 mapbuffer MAPBUFFER;
 
 mapbuffer::mapbuffer() = default;
+mapbuffer::~mapbuffer() = default;
 
-mapbuffer::~mapbuffer()
+void mapbuffer::clear()
 {
-    reset();
-}
-
-void mapbuffer::reset()
-{
-    for( auto &elem : submaps ) {
-        delete elem.second;
-    }
     submaps.clear();
 }
 
-bool mapbuffer::add_submap( const tripoint &p, submap *sm )
+bool mapbuffer::add_submap( const tripoint &p, std::unique_ptr<submap> &sm )
 {
-    if( submaps.count( p ) != 0 ) {
+    if( submaps.count( p ) ) {
         return false;
     }
 
-    submaps[p] = sm;
+    submaps[p] = std::move( sm );
 
     return true;
 }
 
-bool mapbuffer::add_submap( const tripoint &p, std::unique_ptr<submap> &sm )
+bool mapbuffer::add_submap( const tripoint &p, submap *sm )
 {
-    const bool result = add_submap( p, sm.get() );
-    if( result ) {
-        sm.release();
+    // FIXME: get rid of this overload and make submap ownership semantics sane.
+    std::unique_ptr<submap> temp( sm );
+    bool result = add_submap( p, temp );
+    if( !result ) {
+        // NOLINTNEXTLINE( bugprone-unused-return-value )
+        temp.release();
     }
     return result;
 }
@@ -84,7 +80,6 @@ void mapbuffer::remove_submap( tripoint addr )
         debugmsg( "Tried to remove non-existing submap %d,%d,%d", addr.x, addr.y, addr.z );
         return;
     }
-    delete m_target->second;
     submaps.erase( m_target );
 }
 
@@ -102,7 +97,7 @@ submap *mapbuffer::lookup_submap( const tripoint &p )
         return nullptr;
     }
 
-    return iter->second;
+    return iter->second.get();
 }
 
 void mapbuffer::save( bool delete_after_save )
@@ -182,7 +177,7 @@ void mapbuffer::save_quad( const std::string &dirname, const std::string &filena
         submap_addr.x += offsets_offset.x;
         submap_addr.y += offsets_offset.y;
         submap_addrs.push_back( submap_addr );
-        submap *sm = submaps[submap_addr];
+        submap *sm = submaps[submap_addr].get();
         if( sm != nullptr && !sm->is_uniform ) {
             all_uniform = false;
         }
@@ -211,7 +206,7 @@ void mapbuffer::save_quad( const std::string &dirname, const std::string &filena
                 continue;
             }
 
-            submap *sm = submaps[submap_addr];
+            submap *sm = submaps[submap_addr].get();
 
             if( sm == nullptr ) {
                 continue;
@@ -272,7 +267,7 @@ submap *mapbuffer::unserialize_submaps( const tripoint &p )
                   quad_path, p.x, p.y, p.z );
         return nullptr;
     }
-    return submaps[ p ];
+    return submaps[ p ].get();
 }
 
 void mapbuffer::deserialize( JsonIn &jsin )

src/mapbuffer.h

@@ -28,7 +28,7 @@ class mapbuffer
         void save( bool delete_after_save = false );
 
         /** Delete all buffered submaps. **/
-        void reset();
+        void clear();
 
         /** Add a new submap to the buffer.
          *
@@ -38,10 +38,10 @@ class mapbuffer
          * is released (set to NULL).
          * @return true if the submap has been stored here. False if there
          * is already a submap with the specified coordinates. The submap
-         * is not stored than and the caller must take of the submap object
-         * on their own (and properly delete it).
+         * is not stored and the given unique_ptr retains ownsership.
          */
         bool add_submap( const tripoint &p, std::unique_ptr<submap> &sm );
+        // Old overload that we should stop using, but it's complicated
         bool add_submap( const tripoint &p, submap *sm );
 
         /** Get a submap stored in this buffer.
@@ -55,7 +55,7 @@ class mapbuffer
         submap *lookup_submap( const tripoint &p );
 
     private:
-        using submap_map_t = std::map<tripoint, submap *>;
+        using submap_map_t = std::map<tripoint, std::unique_ptr<submap>>;
 
     public:
         inline submap_map_t::iterator begin() {

src/trait_group.cpp

@@ -276,7 +276,6 @@ void Trait_group_collection::add_entry( std::unique_ptr<Trait_creation_data> ptr
     ptr->probability = std::min( 100, ptr->probability );
 
     creators.push_back( std::move( ptr ) );
-    ptr.release();
 }
 
 void Trait_group_distribution::add_entry( std::unique_ptr<Trait_creation_data> ptr )
@@ -289,7 +288,6 @@ void Trait_group_distribution::add_entry( std::unique_ptr<Trait_creation_data> p
     sum_prob += ptr->probability;
 
     creators.push_back( std::move( ptr ) );
-    ptr.release();
 }
 
 Trait_list Trait_group_distribution::create( RecursionList &rec ) const