opentomb / OpenTomb

An open-source Tomb Raider 1-5 engine remake
http://opentomb.github.io/
GNU Lesser General Public License v3.0
1.4k stars 146 forks source link

Change switch-case for std::pair in controls.cpp #603

Open bufalo1973 opened 3 years ago

bufalo1973 commented 3 years ago

I think it would be better to change the switch in void Controls_ActionToStr(char buff[128], ACTIONS act) to something like:

include

std::pair<ACTIONS, std::string> ActionStr[] = { std::make_pair(ACT_UP, "ACT_UP"), std::make_pair(ACT_DOWN, "ACT_DOWN"), std::make_pair(ACT_LEFT, "ACT_LEFT"), std::make_pair(ACT_RIGHT, "ACT_RIGHT"), std::make_pair(ACT_ACTION, "ACT_ACTION"), std::make_pair(ACT_JUMP, "ACT_JUMP"), std::make_pair(ACT_ROLL, "ACT_ROLL"), std::make_pair(ACT_DRAWWEAPON, "ACT_DRAWWEAPON"), std::make_pair(ACT_LOOK, "ACT_LOOK"), std::make_pair(ACT_WALK, "ACT_WALK"), std::make_pair(ACT_SPRINT, "ACT_SPRINT"), std::make_pair(ACT_CROUCH, "ACT_CROUCH"), std::make_pair(ACT_STEPLEFT, "ACT_STEPLEFT"), std::make_pair(ACT_STEPRIGHT, "ACT_STEPRIGHT"), std::make_pair(ACT_LOOKUP, "ACT_LOOKUP"), std::make_pair(ACT_LOOKDOWN, "ACT_LOOKDOWN"), std::make_pair(ACT_LOOKLEFT, "ACT_LOOKLEFT"), std::make_pair(ACT_LOOKRIGHT, "ACT_LOOKRIGHT"), std::make_pair(ACT_NEXTWEAPON, "ACT_NEXTWEAPON"), std::make_pair(ACT_PREVWEAPON, "ACT_PREVWEAPON"), std::make_pair(ACT_FLARE, "ACT_FLARE"), std::make_pair(ACT_BIGMEDI, "ACT_BIGMEDI"), std::make_pair(ACT_SMALLMEDI, "ACT_SMALLMEDI"), std::make_pair(ACT_WEAPON1, "ACT_WEAPON1"), std::make_pair(ACT_WEAPON2, "ACT_WEAPON2"), std::make_pair(ACT_WEAPON3, "ACT_WEAPON3"), std::make_pair(ACT_WEAPON4, "ACT_WEAPON4"), std::make_pair(ACT_WEAPON5, "ACT_WEAPON5"), std::make_pair(ACT_WEAPON6, "ACT_WEAPON6"), std::make_pair(ACT_WEAPON7, "ACT_WEAPON7"), std::make_pair(ACT_WEAPON8, "ACT_WEAPON8"), std::make_pair(ACT_WEAPON9, "ACT_WEAPON9"), std::make_pair(ACT_WEAPON10, "ACT_WEAPON10"), std::make_pair(ACT_BINOCULARS, "ACT_BINOCULARS"), std::make_pair(ACT_PLS, "ACT_PLS"), std::make_pair(ACT_PAUSE, "ACT_PAUSE"), std::make_pair(ACT_INVENTORY, "ACT_INVENTORY"), std::make_pair(ACT_DIARY, "ACT_DIARY"), std::make_pair(ACT_MAP, "ACT_MAP"), std::make_pair(ACT_LOADGAME, "ACT_LOADGAME"), std::make_pair(ACT_SAVEGAME, "ACT_SAVEGAME"), std::make_pair(ACT_CONSOLE, "ACT_CONSOLE"), std::make_pair(ACT_SCREENSHOT, "ACT_SCREENSHOT"), std::make_pair(ACT_LASTINDEX, (std::string)NULL) };

void Controls_ActionToStr(char buff[128], ACTIONS act) { strncpy(buff, ActionStr[act].second.c_str(), 128); };

It might be a little faster.

stohrendorf commented 3 years ago
  1. please use code markup
  2. the std::make_pair(ACT_WEAPON9, "ACT_WEAPON9"), etc. can be replaced with a temporary macro like #define ENUM_MEMBER(name) std::make_pair(name, #name)
  3. you can't construct an std::string from a nullptr
  4. using an array means you rely upon ACTIONS being contiguous and starting from 0
  5. you also need to ensure that the ordering of the members matches exactly the enum members
  6. ... unless you meant to write a loop in Controls_ActionToStr; in this case, ACT_LASTINDEX should be the default if you don't find a matching entry in the array
  7. or use a plain std::map<ACTION, std::string> which you actually want to do here, but performance of small maps (less than ~100 elements) is far inferior to linear scans
  8. std::strncpy does not guarantee that it will create a NUL-terminated string if the source (excluding the terminator) has the same length or is larger than the destination, so you'd need to assert that the source length is less than 128