From 4ed4d17a0cfe60fc28adb4c69e0427d2bf3a31ce Mon Sep 17 00:00:00 2001 From: jove Date: Sun, 21 May 2023 17:09:00 +0100 Subject: [PATCH 1/2] Activity fix --- src/player_activity.cpp | 32 +++++++++++++++++++++++++++++++- src/player_activity.h | 12 ++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/player_activity.cpp b/src/player_activity.cpp index cb385ee3ac58..2ad71dc933db 100644 --- a/src/player_activity.cpp +++ b/src/player_activity.cpp @@ -61,6 +61,17 @@ player_activity::player_activity( std::unique_ptr &&actor ) : ty player_activity::~player_activity() = default; + +void player_activity::resolve_active() +{ + if( active ) { + active = false; + } else { + delete this; + } +} + + void player_activity::migrate_item_position( Character &guy ) { const bool simple_action_replace = @@ -273,6 +284,7 @@ void player_activity::start_or_resume( Character &who, bool resuming ) void player_activity::do_turn( player &p ) { + active = true; // Should happen before activity or it may fail du to 0 moves if( *this && type->will_refuel_fires() ) { try_fuel_fire( *this, p ); @@ -317,6 +329,7 @@ void player_activity::do_turn( player &p ) // be still unloaded, can cause infinite loops. set_to_null(); p.drop_invalid_inventory(); + resolve_active(); return; } const bool travel_activity = id() == activity_id( "ACT_TRAVELLING" ); @@ -344,6 +357,7 @@ void player_activity::do_turn( player &p ) ( activity_id( "ACT_WAIT_STAMINA" ), to_moves( 1_minutes ) ); new_act->values.push_back( 200 + p.get_stamina_max() / 3 ); p.assign_activity( std::move( new_act ) ); + resolve_active(); return; } if( *this && type->rooted() ) { @@ -372,6 +386,7 @@ void player_activity::do_turn( player &p ) // handle it, drop any overflow that may have caused p.drop_invalid_inventory(); } + resolve_active(); } void player_activity::canceled( Character &who ) @@ -467,13 +482,28 @@ activity_ptr::activity_ptr() : act( std::make_unique() ) {} activity_ptr::activity_ptr( activity_ptr && ) = default; activity_ptr::activity_ptr( std::unique_ptr &&source ) { + check_active(); act = std::move( source ); } activity_ptr &activity_ptr::operator=( activity_ptr && ) = default; activity_ptr &activity_ptr::operator=( std::unique_ptr &&source ) { + check_active(); act = std::move( source ); return *this; } -activity_ptr::~activity_ptr() = default; +activity_ptr::~activity_ptr() +{ + check_active(); +}; + +void activity_ptr::check_active() +{ + if( act && act->active ) { + //If the activity is active then we're currently inside it's do_turn so it's not safe to delete it. + //It will delete itself at the end of it's do_turn function. + act->active = false; + act.release(); + } +} diff --git a/src/player_activity.h b/src/player_activity.h index 1fd1d8907624..0a2895992f70 100644 --- a/src/player_activity.h +++ b/src/player_activity.h @@ -28,6 +28,7 @@ class avatar; class monster; class player; class translation; +class activity_ptr; class player_activity { @@ -37,6 +38,13 @@ class player_activity std::set ignored_distractions; + friend activity_ptr; + /** This keeps track of if the activity is currently running so we can avoid deleting it until it's done. */ + bool active = false; + + /** Unlocks the activity, or deletes it if it's already gone. */ + void resolve_active(); + public: /** Total number of moves required to complete the activity */ int moves_total = 0; @@ -69,6 +77,7 @@ class player_activity */ bool auto_resume = false; + player_activity(); // This constructor does not work with activites using the new activity_actor system // TODO: delete this constructor once migration to the activity_actor system is complete @@ -162,6 +171,9 @@ class activity_ptr { private: std::unique_ptr act; + + /** This releases the activity's memory if it's still active, it will now manage it's own memory */ + void check_active(); public: activity_ptr(); activity_ptr( const activity_ptr & ) = delete; From a05a51cd9119321690949874eefe61d1f7db06fa Mon Sep 17 00:00:00 2001 From: jove Date: Mon, 22 May 2023 00:39:31 +0100 Subject: [PATCH 2/2] More safety --- src/armor_layers.cpp | 16 +-- src/character.cpp | 12 +- src/character.h | 6 +- src/inventory_ui.cpp | 22 +++ src/item_contents.cpp | 4 +- src/item_contents.h | 2 +- src/item_stack.h | 12 +- src/location_vector.cpp | 286 ++++++++++++++++++++++++++++++++------ src/location_vector.h | 238 ++++++++++++++++++++++++++++--- src/map.cpp | 36 +---- src/map.h | 9 +- src/monster.cpp | 2 +- src/monster.h | 2 +- src/panels.cpp | 5 +- src/vehicle.cpp | 37 +---- src/vehicle.h | 4 - tests/drop_token_test.cpp | 10 +- 17 files changed, 519 insertions(+), 184 deletions(-) diff --git a/src/armor_layers.cpp b/src/armor_layers.cpp index a16ec9e00498..42758a5553b4 100644 --- a/src/armor_layers.cpp +++ b/src/armor_layers.cpp @@ -81,7 +81,7 @@ struct item_penalties { }; // Figure out encumbrance penalties this clothing is involved in -item_penalties get_item_penalties( ItemList::const_iterator worn_item_it, +item_penalties get_item_penalties( location_vector::const_iterator worn_item_it, const Character &c, int tabindex ) { item *const &worn_item = *worn_item_it; @@ -166,7 +166,7 @@ std::string body_part_names( const std::vector &parts ) } void draw_mid_pane( const catacurses::window &w_sort_middle, - ItemList::const_iterator const worn_item_it, + location_vector::const_iterator const worn_item_it, const Character &c, int tabindex ) { item *const &worn_item = *worn_item_it; @@ -490,7 +490,7 @@ void show_armor_layers_ui( Character &who ) int leftListLines = 0; int rightListLines = 0; - std::vector tmp_worn; + std::vector::iterator> tmp_worn; std::array armor_cat = {{ _( "Torso" ), _( "Head" ), _( "Eyes" ), _( "Mouth" ), _( "L. Arm" ), _( "R. Arm" ), _( "L. Hand" ), _( "R. Hand" ), _( "L. Leg" ), _( "R. Leg" ), _( "L. Foot" ), @@ -823,16 +823,12 @@ void show_armor_layers_ui( Character &who ) } } } else if( action == "SORT_ARMOR" ) { - // Copy to a vector because stable_sort requires random-access - // iterators - - std::vector worn_copy( who.worn.begin(), who.worn.end() ); - std::stable_sort( worn_copy.begin(), worn_copy.end(), + std::stable_sort( who.worn.begin(), + who.worn.end(), []( item * const & l, item * const & r ) { return l->get_layer() < r->get_layer(); } ); - std::copy( worn_copy.begin(), worn_copy.end(), who.worn.begin() ); who.reset_encumbrance(); } else if( action == "EQUIP_ARMOR" ) { // filter inventory for all items that are armor/clothing @@ -873,7 +869,7 @@ void show_armor_layers_ui( Character &who ) if( loc ) { // wear the item loc->obtain( who ); - ItemList::iterator cursor_it = tmp_worn[leftListIndex]; + location_vector::iterator cursor_it = tmp_worn[leftListIndex]; if( !who.as_player()->wear_possessed( *loc, true, cursor_it ) && who.is_npc() ) { // TODO: Pass the reason here popup( _( "Can't put this on!" ) ); diff --git a/src/character.cpp b/src/character.cpp index d4961ed4fdbe..1336daec1b88 100644 --- a/src/character.cpp +++ b/src/character.cpp @@ -2315,7 +2315,7 @@ int Character::get_mod_stat_from_bionic( const character_stat &Stat ) const } detached_ptr Character::wear_item( detached_ptr &&wear, - bool interactive, std::optional::iterator> position ) + bool interactive, std::optional::iterator> position ) { if( !wear ) { return std::move( wear ); @@ -2334,7 +2334,7 @@ detached_ptr Character::wear_item( detached_ptr &&wear, last_item = to_wear.typeId(); - std::vector::iterator pos = position ? *position : position_to_wear_new_item( to_wear ); + location_vector::iterator pos = position ? *position : position_to_wear_new_item( to_wear ); worn.insert( pos, std::move( wear ) ); if( interactive ) { @@ -2385,7 +2385,7 @@ void Character::add_worn( detached_ptr &&wear ) return; } item &to_wear = *wear; - std::vector::iterator pos = position_to_wear_new_item( to_wear ); + location_vector::iterator pos = position_to_wear_new_item( to_wear ); worn.insert( pos, std::move( wear ) ); to_wear.on_wear( *this ); inv.update_invlet( to_wear ); @@ -3279,7 +3279,7 @@ ret_val Character::can_wear( const item &it, bool with_equip_change ) cons } bool Character::wear_possessed( item &to_wear, bool interactive, - std::optional::iterator> position ) + std::optional::iterator> position ) { if( is_worn( to_wear ) ) { if( interactive ) { @@ -4260,7 +4260,7 @@ int layer_details::layer( const int encumbrance ) return total - current; } -ItemList::iterator Character::position_to_wear_new_item( const item &new_item ) +location_vector::iterator Character::position_to_wear_new_item( const item &new_item ) { // By default we put this item on after the last item on the same or any // lower layer. @@ -4296,7 +4296,7 @@ void Character::item_encumb( char_encumbrance_data &vals, const item &new_item ) vals = char_encumbrance_data(); // Figure out where new_item would be worn - ItemList::const_iterator new_item_position = worn.end(); + location_vector::const_iterator new_item_position = worn.end(); if( !new_item.is_null() ) { // const_cast required to work around g++-4.8 library bug // see the commit that added this comment to understand why diff --git a/src/character.h b/src/character.h index 5d6d16309256..d5b4da686c8f 100644 --- a/src/character.h +++ b/src/character.h @@ -854,7 +854,7 @@ class Character : public Creature, public location_visitable /** Return the position in the worn list where new_item would be * put by default */ - ItemList::iterator position_to_wear_new_item( const item &new_item ); + location_vector::iterator position_to_wear_new_item( const item &new_item ); /** Applies encumbrance from items only * If new_item is not null, then calculate under the asumption that it @@ -1379,7 +1379,7 @@ class Character : public Creature, public location_visitable * @return nullopt on fail, pointer to newly worn item on success */ bool wear_possessed( item &to_wear, bool interactive = true, - std::optional::iterator> position = std::nullopt ); + std::optional::iterator> position = std::nullopt ); /** * Wear a copy of specified item. * @param to_wear Item to wear. Will be moved from if actually worn. @@ -1387,7 +1387,7 @@ class Character : public Creature, public location_visitable * @return the item if it was not worn */ detached_ptr wear_item( detached_ptr &&to_wear, - bool interactive = true, std::optional::iterator> position = std::nullopt ); + bool interactive = true, std::optional::iterator> position = std::nullopt ); /** * Wears an item in its default location with no checks. diff --git a/src/inventory_ui.cpp b/src/inventory_ui.cpp index c2fb7d7db943..99187673c1d7 100644 --- a/src/inventory_ui.cpp +++ b/src/inventory_ui.cpp @@ -1074,6 +1074,28 @@ static std::vector> restack_items( const item_stack::const_ite return res; } +static std::vector> restack_items( const std::vector::const_iterator + &from, + const std::vector::const_iterator &to, bool check_components = false ) +{ + std::vector> res; + + for( auto it = from; it != to; ++it ) { + auto match = std::find_if( res.begin(), res.end(), + [ &it, check_components ]( const std::list &e ) { + return ( *it )->display_stacked_with( *const_cast( e.back() ), check_components ); + } ); + + if( match != res.end() ) { + match->push_back( const_cast( *it ) ); + } else { + res.emplace_back( 1, const_cast( *it ) ); + } + } + + return res; +} + const item_category *inventory_selector::naturalize_category( const item_category &category, const tripoint &pos ) { diff --git a/src/item_contents.cpp b/src/item_contents.cpp index d3ba1a28d2f5..f952b62d0c4b 100644 --- a/src/item_contents.cpp +++ b/src/item_contents.cpp @@ -144,7 +144,7 @@ const std::vector &item_contents::all_items_top() const detached_ptr item_contents::remove_top( item *it ) { - std::vector::iterator iter = std::find_if( items.begin(), + auto iter = std::find_if( items.begin(), items.end(), [&it]( item *&against ) { return against == it; } ); @@ -153,7 +153,7 @@ detached_ptr item_contents::remove_top( item *it ) return ret; } -std::vector::iterator item_contents::remove_top( std::vector::iterator &it, +location_vector::iterator item_contents::remove_top( location_vector::iterator &it, detached_ptr *removed ) { return items.erase( it, removed ); diff --git a/src/item_contents.h b/src/item_contents.h index 36fb427839b2..164a86a4612b 100644 --- a/src/item_contents.h +++ b/src/item_contents.h @@ -36,7 +36,7 @@ class item_contents /** removes a top-level item */ detached_ptr remove_top( item *it ); - std::vector::iterator remove_top( std::vector::iterator &it, + location_vector::iterator remove_top( location_vector::iterator &it, detached_ptr *removed = nullptr ); // returns a list of pointers to all items inside recursively diff --git a/src/item_stack.h b/src/item_stack.h index 11a59d31a729..a4b1342cdb53 100644 --- a/src/item_stack.h +++ b/src/item_stack.h @@ -22,10 +22,10 @@ class item_stack location_vector *items; public: - using iterator = std::vector::iterator; - using const_iterator = std::vector::const_iterator; - using reverse_iterator = std::vector::reverse_iterator; - using const_reverse_iterator = std::vector::const_reverse_iterator; + using iterator = location_vector::iterator; + using const_iterator = location_vector::const_iterator; + using reverse_iterator = location_vector::reverse_iterator; + using const_reverse_iterator = location_vector::const_reverse_iterator; item_stack( location_vector *items ) : items( items ) { } virtual ~item_stack() = default; @@ -34,8 +34,8 @@ class item_stack bool empty() const; virtual void insert( detached_ptr &&newitem ) = 0; virtual iterator erase( const_iterator it, detached_ptr *out = nullptr ) = 0; - virtual iterator erase( const_iterator first, const_iterator last, - std::vector> *out = nullptr ) = 0; + /*virtual iterator erase( const_iterator first, const_iterator last, + std::vector> *out = nullptr ) = 0;*/ virtual std::vector> clear(); void remove_top_items_with( std::function < detached_ptr( detached_ptr && ) > cb ); diff --git a/src/location_vector.cpp b/src/location_vector.cpp index fe87d4454913..ea9686b3e970 100644 --- a/src/location_vector.cpp +++ b/src/location_vector.cpp @@ -25,9 +25,181 @@ location_vector::~location_vector() on_destroy(); } +template +location_vector::iterator::iterator() = default; + +template +location_vector::iterator::iterator( typename std::vector::iterator it, + const location_vector &home ) : it( it ), home( &home ) +{ + this->home->locked++; +}; + +template +location_vector::iterator::iterator( const location_vector::iterator &source ) +{ + this->it = source.it; + this->home = source.home; + if( this->home ) { + this->home->locked++; + } +}; + +template +location_vector::iterator::iterator( location_vector::iterator &&source ) +{ + this->it = source.it; + this->home = source.home; + source.home = nullptr; + source.it = {}; +}; + +template +typename location_vector::iterator &location_vector::iterator::operator=( const + location_vector::iterator &source ) +{ + release_locked(); + this->it = source.it; + this->home = source.home; + if( this->home ) { + this->home->locked++; + } + return *this; +}; + +template +typename location_vector::iterator &location_vector::iterator::operator= +( location_vector::iterator &&source ) +{ + release_locked(); + this->it = source.it; + this->home = source.home; + source.home = nullptr; + source.it = {}; + return *this; +}; + +template +location_vector::iterator::~iterator() +{ + release_locked(); +}; + + +template +void location_vector::iterator::release_locked() +{ + + if( this->home ) { + this->home->locked--; + if( this->home->locked < 0 ) { + debugmsg( "Location vector's locked is negative" ); + } + } +} + +template +location_vector::const_iterator::const_iterator( const iterator &source ) +{ + this->it = source.it; + this->home = source.home; + if( this->home ) { + this->home->locked++; + } +} + +template +location_vector::const_iterator::const_iterator( iterator &&source ) +{ + this->it = source.it; + this->home = source.home; + source.home = nullptr; + source.it = {}; +} + +template +location_vector::const_iterator::const_iterator() = default; + +template +location_vector::const_iterator::const_iterator( typename std::vector::const_iterator it, + const location_vector &home ) : it( it ), home( &home ) +{ + this->home->locked++; +}; + +template +location_vector::const_iterator::const_iterator( const location_vector::const_iterator + &source ) +{ + this->it = source.it; + this->home = source.home; + if( this->home ) { + this->home->locked++; + } +}; + +template +location_vector::const_iterator::const_iterator( location_vector::const_iterator &&source ) +{ + this->it = source.it; + this->home = source.home; + source.home = nullptr; + source.it = {}; +}; + +template +typename location_vector::const_iterator &location_vector::const_iterator::operator= +( const location_vector::const_iterator &source ) +{ + release_locked(); + this->it = source.it; + this->home = source.home; + if( this->home ) { + this->home->locked++; + } + return *this; +}; + +template +typename location_vector::const_iterator &location_vector::const_iterator::operator= +( location_vector::const_iterator &&source ) +{ + release_locked(); + this->it = source.it; + this->home = source.home; + source.home = nullptr; + source.it = {}; + return *this; +}; + +template +location_vector::const_iterator::~const_iterator() +{ + release_locked(); +}; + + +template +void location_vector::const_iterator::release_locked() +{ + + if( this->home ) { + this->home->locked--; + if( this->home->locked < 0 ) { + debugmsg( "Location vector's locked is negative" ); + } + } +} + + + template void location_vector::push_back( detached_ptr &&obj ) { + if( locked > 0 ) { + debugmsg( "Attempting to push_back to a vector with active iterators" ); + return; + } if( !obj ) { return; } @@ -68,6 +240,10 @@ T *location_vector::back() const template detached_ptr location_vector::remove( T *obj ) { + if( locked > 0 ) { + debugmsg( "Attempting to remove something from a vector with active iterators" ); + return detached_ptr(); + } if( destroyed ) { debugmsg( "Attempted to remove something from a destroyed location." ); return detached_ptr(); @@ -76,7 +252,7 @@ detached_ptr location_vector::remove( T *obj ) for( auto iter = contents.begin(); iter != contents.end(); iter++ ) { if( *iter == obj ) { detached_ptr ret; - erase( iter, &ret ); + erase( location_vector::iterator( iter, *this ), &ret ); return ret; } } @@ -91,31 +267,38 @@ const std::vector &location_vector::as_vector() const } template -typename std::vector::iterator location_vector::erase( typename - std::vector::const_iterator - it, +typename location_vector::iterator location_vector::erase( typename + location_vector::const_iterator it, detached_ptr *out ) { + if( locked > 2 ) { + debugmsg( "Attempting to erase something from a vector with more than 1 active iterator" ); + return location_vector::iterator( contents.end(), *this ); + } if( destroyed && out ) { debugmsg( "Attempted to erase something from a destroyed location." ); - return contents.end(); + return location_vector::iterator( contents.end(), *this ); } T *subject = *it; - typename std::vector::iterator ret = contents.erase( it ); + typename std::vector::iterator ret = contents.erase( it.it ); subject->remove_location(); detached_ptr local; detached_ptr *used = out ? out : &local; *used = detached_ptr( subject ); - return ret; + return location_vector::iterator( ret, *this ); } template -typename std::vector::iterator location_vector::insert( typename std::vector::iterator - it, +typename location_vector::iterator location_vector::insert( typename + location_vector::iterator it, detached_ptr &&obj ) { + if( locked > 2 ) { + debugmsg( "Attempting to insert something into a vector with more than 1 active iterator" ); + return it; + } if( !obj ) { return it; } @@ -127,21 +310,24 @@ typename std::vector::iterator location_vector::insert( typename std::ve if( destroyed ) { raw->destroy_in_place(); } - return contents.insert( it, raw ); + return location_vector::iterator( contents.insert( it.it, raw ), *this ); } else { raw->saved_loc = nullptr; raw->set_location( &*loc ); - return std::find( contents.begin(), contents.end(), raw ); + return location_vector::iterator( std::find( contents.begin(), contents.end(), raw ), *this ); } } template -typename std::vector::iterator location_vector::insert( typename std::vector::iterator - it, +typename location_vector::iterator location_vector::insert( typename + location_vector::iterator it, typename std::vector>::iterator start, typename std::vector>::iterator end ) { - + if( locked > 2 ) { + debugmsg( "Attempting to insert something into a vector with more than 1 active iterator" ); + return it; + } for( auto iter = start; iter != end; iter++ ) { if( !*iter ) { continue; @@ -154,7 +340,7 @@ typename std::vector::iterator location_vector::insert( typename std::ve if( destroyed ) { raw->destroy_in_place(); } - it = contents.insert( it, raw ); + it = location_vector::iterator( contents.insert( it.it, raw ), *this ); } else { raw->resolve_saved_loc(); raw->set_location( &*loc ); @@ -164,75 +350,81 @@ typename std::vector::iterator location_vector::insert( typename std::ve } template -typename std::vector::const_iterator location_vector::begin() const +typename location_vector::const_iterator location_vector::begin() const { - return contents.begin(); + return location_vector::const_iterator( contents.cbegin(), *this ); } template -typename std::vector::const_iterator location_vector::end() const +typename location_vector::const_iterator location_vector::end() const { - return contents.end(); + return location_vector::const_iterator( contents.cend(), *this ); } template -typename std::vector::iterator location_vector::begin() +typename location_vector::iterator location_vector::begin() { - return contents.begin(); + return location_vector::iterator( contents.begin(), *this ); } template -typename std::vector::iterator location_vector::end() +typename location_vector::iterator location_vector::end() { - return contents.end(); + return location_vector::iterator( contents.end(), *this ); } template -typename std::vector::const_reverse_iterator location_vector::rbegin() const +typename location_vector::const_reverse_iterator location_vector::rbegin() const { - return contents.rbegin(); + return location_vector::const_reverse_iterator( location_vector::const_iterator( + contents.rbegin().base(), *this ) ); } template -typename std::vector::const_reverse_iterator location_vector::rend() const +typename location_vector::const_reverse_iterator location_vector::rend() const { - return contents.rend(); + return location_vector::const_reverse_iterator( location_vector::const_iterator( + contents.rend().base(), *this ) ); } template -typename std::vector::reverse_iterator location_vector::rbegin() +typename location_vector::reverse_iterator location_vector::rbegin() { - return contents.rbegin(); + return location_vector::reverse_iterator( location_vector::iterator( contents.rbegin().base(), + *this ) ); } template -typename std::vector::reverse_iterator location_vector::rend() +typename location_vector::reverse_iterator location_vector::rend() { - return contents.rend(); + return location_vector::reverse_iterator( location_vector::iterator( contents.rend().base(), + *this ) ); } template -typename std::vector::const_iterator location_vector::cbegin() const +typename location_vector::const_iterator location_vector::cbegin() const { - return contents.cbegin(); + return location_vector::const_iterator( contents.cbegin(), *this ); } template -typename std::vector::const_iterator location_vector::cend() const +typename location_vector::const_iterator location_vector::cend() const { - return contents.cend(); + return location_vector::const_iterator( contents.cend(), *this ); } template -typename std::vector::const_reverse_iterator location_vector::crbegin() const +typename location_vector::const_reverse_iterator location_vector::crbegin() const { - return contents.crbegin(); + return location_vector::const_reverse_iterator( location_vector::const_iterator( + contents.crbegin().base(), *this ) ); } template -typename std::vector::const_reverse_iterator location_vector::crend() const +typename location_vector::const_reverse_iterator location_vector::crend() const { - return contents.crend(); + return location_vector::const_reverse_iterator( location_vector::const_iterator( + contents.crend().base(), *this ) ); } template @@ -250,6 +442,10 @@ location *location_vector::get_location() const template typename std::vector> location_vector::clear() { + if( locked > 0 ) { + debugmsg( "Attempting to clear a vector with active iterators" ); + return std::vector>(); + } if( destroyed ) { debugmsg( "Attempted to clear a destroyed location." ); return std::vector>(); @@ -266,15 +462,15 @@ typename std::vector> location_vector::clear() template void location_vector::remove_with( std::function < detached_ptr( detached_ptr && ) > cb ) { - if( destroyed ) { - debugmsg( "Attempted to remove_with from a destroyed location." ); + if( locked > 0 ) { + debugmsg( "Attempting to clear a vector with active iterators" ); return; } - if( locked ) { - debugmsg( "Recursive removal in location_vector" ); + if( destroyed ) { + debugmsg( "Attempted to remove_with from a destroyed location." ); return; } - locked = true; + locked++; for( auto it = contents.begin(); it != contents.end(); ) { location *saved_loc = ( *it )->loc; ( *it )->remove_location(); @@ -292,7 +488,7 @@ void location_vector::remove_with( std::function < detached_ptr( detached_ it = contents.erase( it ); } } - locked = false; + locked--; } template diff --git a/src/location_vector.h b/src/location_vector.h index 662bb757b48e..81fd7f39bae2 100644 --- a/src/location_vector.h +++ b/src/location_vector.h @@ -12,15 +12,208 @@ class location; struct tripoint; + template class location_vector { private: std::unique_ptr> loc; std::vector contents; - bool locked = false; + mutable int locked = 0; bool destroyed = false; + + public: + struct const_iterator; + struct iterator { + public: + friend const_iterator; + friend location_vector; + using iterator_category = std::random_access_iterator_tag; + using difference_type = std::ptrdiff_t; + using value_type = T*; + using pointer = T **; + using reference = T *&; + + iterator( ); + iterator( typename std::vector::iterator it, const location_vector &home ); + iterator( const iterator &source ); + iterator( iterator &&source ); + iterator &operator=( const iterator &source ); + iterator &operator=( iterator &&source ); + ~iterator(); + + reference operator*() const { + return *it; + } + pointer operator->() { + return &*it; + } + + iterator &operator++() { + it++; + return *this; + } + + iterator &operator--() { + it--; + return *this; + } + + iterator operator++( int ) { + iterator tmp = *this; + ++( *this ); + return tmp; + } + iterator operator--( int ) { + iterator tmp = *this; + --( *this ); + return tmp; + } + + iterator &operator+=( difference_type t ) { + it += t; + return *this; + } + + difference_type operator-( const iterator &rhs ) const { + return it - rhs.it; + } + + friend iterator operator+( difference_type n, const iterator &term ) { + return term + n; + } + + friend iterator operator+( const iterator &term, difference_type n ) { + return iterator( term.it + n, *term.home ); + } + + friend iterator operator-( const iterator &term, difference_type n ) { + return iterator( term.it - n, *term.home ); + } + + friend bool operator== ( const iterator &a, const iterator &b ) { + return a.it == b.it; + }; + friend bool operator!= ( const iterator &a, const iterator &b ) { + return a.it != b.it; + }; + + friend bool operator< ( const iterator &a, const iterator &b ) { + return a.it < b.it; + }; + friend bool operator<= ( const iterator &a, const iterator &b ) { + return a.it <= b.it; + }; + friend bool operator> ( const iterator &a, const iterator &b ) { + return a.it > b.it; + }; + friend bool operator>= ( const iterator &a, const iterator &b ) { + return a.it >= b.it; + }; + + private: + void release_locked(); + typename std::vector::iterator it; + const location_vector *home = nullptr; + }; + + struct const_iterator { + public: + friend iterator; + friend location_vector; + using iterator_category = std::random_access_iterator_tag; + using difference_type = std::ptrdiff_t; + using value_type = T * const; + using pointer = T * const*; + using reference = T * const&; + + const_iterator( ); + const_iterator( typename std::vector::const_iterator it, const location_vector &home ); + const_iterator( const iterator &source ); + const_iterator( iterator &&source ); + const_iterator( const const_iterator &source ); + const_iterator( const_iterator &&source ); + const_iterator &operator=( const const_iterator &source ); + const_iterator &operator=( const_iterator &&source ); + ~const_iterator(); + + reference operator*() const { + return *it; + } + pointer operator->() { + return &*it; + } + + const_iterator &operator++() { + it++; + return *this; + } + + const_iterator &operator--() { + it--; + return *this; + } + + const_iterator operator++( int ) { + const_iterator tmp = *this; + ++( *this ); + return tmp; + } + const_iterator operator--( int ) { + const_iterator tmp = *this; + --( *this ); + return tmp; + } + + const_iterator &operator+=( difference_type t ) { + it += t; + return *this; + } + + difference_type operator-( const const_iterator &rhs ) const { + return it - rhs.it; + } + + friend const_iterator operator+( difference_type n, const const_iterator &term ) { + return term + n; + } + + friend const_iterator operator+( const const_iterator &term, difference_type n ) { + return const_iterator( term.it + n, term.home ); + } + + friend const_iterator operator-( const const_iterator &term, difference_type n ) { + return const_iterator( term.it - n, *term.home ); + } + + friend bool operator== ( const const_iterator &a, const const_iterator &b ) { + return a.it == b.it; + }; + friend bool operator!= ( const const_iterator &a, const const_iterator &b ) { + return a.it != b.it; + }; + + friend bool operator< ( const const_iterator &a, const const_iterator &b ) { + return a.it < b.it; + }; + friend bool operator<= ( const const_iterator &a, const const_iterator &b ) { + return a.it <= b.it; + }; + friend bool operator> ( const const_iterator &a, const const_iterator &b ) { + return a.it > b.it; + }; + friend bool operator>= ( const const_iterator &a, const const_iterator &b ) { + return a.it >= b.it; + }; + + private: + typename std::vector::const_iterator it; + const location_vector *home; + void release_locked(); + }; + using reverse_iterator = std::reverse_iterator; + using const_reverse_iterator = std::reverse_iterator; location_vector( location *loc ); location_vector( location *loc, std::vector> &from ); @@ -39,28 +232,27 @@ class location_vector detached_ptr remove( T * ); const std::vector &as_vector() const; - typename std::vector::iterator erase( typename std::vector::const_iterator it, - detached_ptr *out = nullptr ); - - typename std::vector::iterator insert( typename std::vector::iterator it, - detached_ptr &&obj ); - - typename std::vector::iterator insert( typename std::vector::iterator it, - typename std::vector>::iterator start, - typename std::vector>::iterator end ); - typename std::vector::const_iterator begin() const; - typename std::vector::const_iterator end() const; - typename std::vector::iterator begin(); - typename std::vector::iterator end(); - typename std::vector::const_reverse_iterator rbegin() const; - typename std::vector::const_reverse_iterator rend() const; - typename std::vector::const_reverse_iterator crbegin() const; - typename std::vector::const_reverse_iterator crend() const; - typename std::vector::reverse_iterator rbegin(); - typename std::vector::reverse_iterator rend(); - typename std::vector::const_iterator cbegin() const; - typename std::vector::const_iterator cend() const; - typename std::vector> clear(); + iterator erase( const_iterator it, + detached_ptr *out = nullptr ); + iterator insert( iterator it, + detached_ptr &&obj ); + + iterator insert( iterator it, + typename std::vector>::iterator start, + typename std::vector>::iterator end ); + const_iterator begin() const; + const_iterator end() const; + iterator begin(); + iterator end(); + const_reverse_iterator rbegin() const; + const_reverse_iterator rend() const; + const_reverse_iterator crbegin() const; + const_reverse_iterator crend() const; + reverse_iterator rbegin(); + reverse_iterator rend(); + const_iterator cbegin() const; + const_iterator cend() const; + std::vector> clear(); void remove_with( std::function < detached_ptr( detached_ptr && ) > cb ); diff --git a/src/map.cpp b/src/map.cpp index 558e249905e0..de74b03e2b0d 100644 --- a/src/map.cpp +++ b/src/map.cpp @@ -153,14 +153,7 @@ map &get_map() // Map stack methods. map_stack::iterator map_stack::erase( map_stack::const_iterator it, detached_ptr *ret ) { - return myorigin->i_rem( location, it, ret ); -} - -map_stack::iterator map_stack::erase( map_stack::const_iterator first, - map_stack::const_iterator last, - std::vector> *ret ) -{ - return myorigin->i_rem( location, first, last, ret ); + return myorigin->i_rem( location, std::move( it ), ret ); } void map_stack::insert( detached_ptr &&newitem ) @@ -4198,32 +4191,7 @@ map_stack::iterator map::i_rem( const tripoint &p, map_stack::const_iterator it, current_submap->update_lum_rem( l, **it ); - return current_submap->get_items( l ).erase( it, out ); -} - -map_stack::iterator map::i_rem( const tripoint &p, map_stack::const_iterator first, - map_stack::const_iterator last, std::vector> *out ) -{ - point l; - submap *const current_submap = get_submap_at( p, l ); - map_stack::iterator ret = current_submap->get_items( l ).end(); - while( first != last ) { - // remove from the active items cache (if it isn't there does nothing) - current_submap->active_items.remove( *first ); - if( current_submap->active_items.empty() ) { - submaps_with_active_items.erase( tripoint( abs_sub.x + p.x / SEEX, abs_sub.y + p.y / SEEY, p.z ) ); - } - - current_submap->update_lum_rem( l, **first ); - - detached_ptr local; - ret = current_submap->get_items( l ).erase( first, &local ); - if( out ) { - out->push_back( std::move( local ) ); - } - first++; - } - return ret; + return current_submap->get_items( l ).erase( std::move( it ), out ); } detached_ptr map::i_rem( const tripoint &p, item *it ) diff --git a/src/map.h b/src/map.h index b13b5256f911..a3586d8c8702 100644 --- a/src/map.h +++ b/src/map.h @@ -110,8 +110,6 @@ class map_stack : public item_stack item_stack( newstack ), location( newloc ), myorigin( neworigin ) {} void insert( detached_ptr &&newitem ) override; iterator erase( const_iterator it, detached_ptr *out = nullptr ) override; - iterator erase( const_iterator first, const_iterator last, - std::vector> *out = nullptr ) override; int count_limit() const override { return MAX_ITEM_IN_SQUARE; } @@ -1197,12 +1195,7 @@ class map detached_ptr *out = nullptr ) { return i_rem( tripoint( location, abs_sub.z ), it, out ); } - map_stack::iterator i_rem( const tripoint &p, map_stack::const_iterator first, - map_stack::const_iterator last, std::vector> *out = nullptr ); - map_stack::iterator i_rem( const point &location, map_stack::const_iterator first, - map_stack::const_iterator last, std::vector> *out = nullptr ) { - return i_rem( tripoint( location, abs_sub.z ), first, last, out ); - } + detached_ptr i_rem( const tripoint &p, item *it ); detached_ptr i_rem( point p, item *it ) { return i_rem( tripoint( p, abs_sub.z ), it ); diff --git a/src/monster.cpp b/src/monster.cpp index 4a5b474c1fef..baa94bdc8ddc 100644 --- a/src/monster.cpp +++ b/src/monster.cpp @@ -3255,7 +3255,7 @@ detached_ptr monster::remove_item( item *it ) return ret; } -std::vector::iterator monster::remove_item( std::vector::iterator &it, +location_vector::iterator monster::remove_item( location_vector::iterator &it, detached_ptr *result ) { return inv.erase( it, result ); diff --git a/src/monster.h b/src/monster.h index e869735ce74c..c778e853e979 100644 --- a/src/monster.h +++ b/src/monster.h @@ -437,7 +437,7 @@ class monster : public Creature, public location_visitable const std::vector &get_items() const; detached_ptr remove_item( item *it ); - std::vector::iterator remove_item( std::vector::iterator &it, + location_vector::iterator remove_item( location_vector::iterator &it, detached_ptr *result = nullptr ); std::vector> clear_items(); void drop_items(); diff --git a/src/panels.cpp b/src/panels.cpp index 911be9aef91c..1a3eac2252fe 100644 --- a/src/panels.cpp +++ b/src/panels.cpp @@ -578,11 +578,12 @@ static std::pair temp_stat( const avatar &u ) static std::string get_armor( const avatar &u, body_part bp, unsigned int truncate = 0 ) { - for( ItemList::const_iterator it = u.worn.end(); it != u.worn.begin(); ) { - --it; + for( auto it = u.worn.rbegin(); it != u.worn.rend(); ) { if( ( *it )->covers( bp ) ) { return ( *it )->tname( 1, true, truncate ); } + + it++; } return "-"; } diff --git a/src/vehicle.cpp b/src/vehicle.cpp index 46ca9223f554..4988f2a7b10f 100644 --- a/src/vehicle.cpp +++ b/src/vehicle.cpp @@ -233,13 +233,7 @@ class MapgenRemovePartHandler : public RemovePartHandler vehicle_stack::iterator vehicle_stack::erase( vehicle_stack::const_iterator it, detached_ptr *out ) { - return myorigin->remove_item( part_num, it, out ); -} - -vehicle_stack::iterator vehicle_stack::erase( vehicle_stack::const_iterator first, - vehicle_stack::const_iterator last, std::vector> *out ) -{ - return myorigin->remove_item( part_num, first, last, out ); + return myorigin->remove_item( part_num, std::move( it ), out ); } void vehicle_stack::insert( detached_ptr &&newitem ) @@ -5464,10 +5458,10 @@ detached_ptr vehicle::add_item( int part, detached_ptr &&itm ) detached_ptr vehicle::remove_item( int part, item *it ) { - const std::vector &veh_items = parts[part].items.as_vector(); + const location_vector &veh_items = parts[part].items; //const std::vector::const_iterator iter = veh_items.get_iterator_from_pointer( it ); - const std::vector::const_iterator iter = std::find_if( veh_items.begin(), + const location_vector::const_iterator iter = std::find_if( veh_items.begin(), veh_items.end(), [&it]( const item * const & item ) { return it == item; } ); @@ -5486,34 +5480,11 @@ vehicle_stack::iterator vehicle::remove_item( int part, vehicle_stack::const_ite // remove from the active items cache (if it isn't there does nothing) active_items.remove( *it ); - vehicle_stack::iterator iter = parts[part].items.erase( it, ret ); + vehicle_stack::iterator iter = parts[part].items.erase( std::move( it ), ret ); invalidate_mass(); return iter; } -vehicle_stack::iterator vehicle::remove_item( int part, vehicle_stack::const_iterator first, - vehicle_stack::const_iterator last, std::vector> *out ) -{ - location_vector &veh_items = parts[part].items; - vehicle_stack::iterator ret = veh_items.end(); - while( first != last ) { - - // remove from the active items cache (if it isn't there does nothing) - active_items.remove( *first ); - - if( out ) { - detached_ptr det; - ret = veh_items.erase( first, &det ); - out->push_back( std::move( det ) ); - } else { - ret = veh_items.erase( first ); - } - invalidate_mass(); - first++; - } - return ret; -} - vehicle_stack vehicle::get_items( const int part ) { const tripoint pos = global_part_pos3( part ); diff --git a/src/vehicle.h b/src/vehicle.h index da838f9813fb..73a9beaf1835 100644 --- a/src/vehicle.h +++ b/src/vehicle.h @@ -131,8 +131,6 @@ class vehicle_stack : public item_stack vehicle_stack( location_vector *newstack, point newloc, vehicle *neworigin, int part ) : item_stack( newstack ), location( newloc ), myorigin( neworigin ), part_num( part ) {} iterator erase( const_iterator it, detached_ptr *out = nullptr ) override; - iterator erase( const_iterator first, const_iterator last, - std::vector> *out = nullptr ) override; void insert( detached_ptr &&newitem ) override; int count_limit() const override { return MAX_ITEM_IN_VEHICLE_STORAGE; @@ -1591,8 +1589,6 @@ class vehicle detached_ptr remove_item( int part, item *it ); vehicle_stack::iterator remove_item( int part, vehicle_stack::const_iterator it, detached_ptr *ret = nullptr ); - vehicle_stack::iterator remove_item( int part, vehicle_stack::const_iterator first, - vehicle_stack::const_iterator last, std::vector> *ret = nullptr ); vehicle_stack get_items( int part ) const; vehicle_stack get_items( int part ); diff --git a/tests/drop_token_test.cpp b/tests/drop_token_test.cpp index d0ffb0560fc1..e73b4a839c73 100644 --- a/tests/drop_token_test.cpp +++ b/tests/drop_token_test.cpp @@ -44,11 +44,6 @@ class testing_stack : public item_stack iterator erase( const_iterator it, detached_ptr *out = nullptr ) override { return items->erase( it, out ); } - iterator erase( const_iterator, const_iterator, - std::vector> * = nullptr ) override { - debugmsg( "Unused and dummied out" ); - return items->end(); - } int count_limit() const override { return INT_MAX; } @@ -144,6 +139,8 @@ TEST_CASE( "full backpack drop", "[activity][drop_token]" ) REQUIRE( first_backpack_iter != dummy.worn.end() ); drop.push_back( drop_location( **first_duffel_iter, 1 ) ); drop.push_back( drop_location( **first_backpack_iter, 1 ) ); + + //Clear these lest they trigger safety warnings std::list drop_list = pickup::reorder_for_dropping( dummy, drop ); THEN( "he will try to drop some, but not all of the carried items" ) { REQUIRE( drop_list.size() > 4 ); @@ -198,6 +195,9 @@ TEST_CASE( "full backpack drop", "[activity][drop_token]" ) } } + first_duffel_iter = {}; + first_backpack_iter = {}; + // +1 because we aren't checking tokens, but the last item is still zero cost CHECK( actual_duffel_content_count >= expected_duffel_content_count ); CHECK( actual_duffel_content_count <= expected_duffel_content_count + 1 );