Proposal: WIP: Use type-safe ids #1707

selaux · 2022-11-15T08:10:42Z

This introduces a type-safe ID for Items. It's a struct with a single field, so it should serialize the same as the underlying type, making it usable in save games and such.

Why?

Currently we always use the underlying type (in this case UINT16) or a type alias. This has the disadvantage that the type checker does not check whether what you are using is actually an itemid and not just another UINT16 (or even other types that are automatically converted to UINT16). It also encourages things like iterating over item ids using ++ and strict item id bounds such as MAX_WEAPONS, which limits moddablility (what if I want to have more weapons in my mod).

Continuing the refactor

This would require other entities such as weapons to use the TypedId<> template (see Types.h) as well and refactoring the places where .inner or ItemId(x) is used to use iterators.

Implications

Most places using .inner() and ItemId(x) should be considered technical debt and should be replaced, e.g. by iterators.

Happy for some feedback on this one.

lynxlynxlynx · 2022-11-15T09:57:54Z

Wouldn't it be cleaner to use an enum type? No need for an extra class, while still providing the extra strictness and arbitrary size.

momoko-h · 2022-11-15T10:31:34Z

Yes, I also don't quite see the advantage of this approach over simply changing ITEMDEFINE to an enum class that inherits from UINT16. Since ITEMDEFINE looks fairly ugly you could also change the name, of course. You would have to change every occurrence of JAR, etc. to ItemId::JAR or add constexprs for them, but at least that's more idiomatic than JAR.inner().

One other thing I noticed: we really should give OBJECTTYPE a constructor that simply calls CreateItem(). OBJECTTYPE o; CreateItem(HMX, 100, &o); is so old school C.

selaux · 2022-11-15T17:42:42Z

I disagree. An enum class implies that there is a known number of known variants. I would argue we currently only have the known number and that also only holds as long as we don't allow adding any additional items in mods. What the actual item id points to is known only after loading the items.json. There is no guarantee what ItemId::Jar would point to at runtime. My general standpoint is we shouldn't have any ITEMDEFINEs in C++ at all, although that is more of a long term goal 😉.

lynxlynxlynx · 2022-11-15T20:29:42Z

I wouldn't get stuck on the fact that it's an enum, since it'd clearly be used as a type. Just like nobody would find the underlying ugliness of your class approach, unless they looked specifically. ;P
And we don't actually need to care what underlying values are used for dynamic instances. Nor for the several hardcoded cases if we put some annoying work into it. But these two things are the same for both approaches anyway.

momoko-h · 2022-11-16T10:46:21Z

src/game/Editor/EditorMercs.cc

@@ -2114,7 +2114,7 @@ static void AddNewItemToSelectedMercsInventory(BOOLEAN fCreate)
 		if( !(GCM->getItem(gpSelected->pDetailedPlacement->Inv[ gbMercSlotTypes[ gbCurrSelect ] ].usItem)->isAmmo()) )
 			gpSelected->pDetailedPlacement->Inv[ gbMercSlotTypes[ gbCurrSelect ] ].bStatus[0] = (INT8)(80 + Random( 21 ));

-		if( gusMercsNewItemIndex )
+		if( gusMercsNewItemIndex != ItemId(NOTHING) )


You already defined NOTHING as an ItemId.

momoko-h · 2022-11-16T10:52:02Z

src/game/Laptop/BobbyR.cc

 }


 static void InitBobbyRayUsedInventory(void)
 {
-	UINT16	i;
+	ItemId	i;


I'd use the opportunity to move this into the for init-statement, here and in the other places.

momoko-h · 2022-11-16T10:56:16Z

src/game/SaveLoadGame.cc

@@ -1359,7 +1359,7 @@ static void LoadSoldierStructure(HWFILE const f, UINT32 savegame_version, bool s
 		if (!active) continue;

 		//Read in the saved soldier info into a Temp structure
-		SOLDIERTYPE SavedSoldierInfo;
+		SOLDIERTYPE SavedSoldierInfo = {};


SOLDIERTYPE already has a default constructor because of the ST::strings and other stuff it contains, so this does nothing useful.

momoko-h · 2022-11-16T10:57:30Z

src/game/Strategic/Auto_Resolve.cc

@@ -2533,7 +2533,7 @@ static UINT8 GetUnusedMercProfileID(void)

 static void CreateTempPlayerMerc(void)
 {
-	SOLDIERCREATE_STRUCT		MercCreateStruct;
+	SOLDIERCREATE_STRUCT		MercCreateStruct = {};


The line below is now not needed anymore.

momoko-h · 2022-11-16T11:04:07Z

src/game/Strategic/Quest_Debug_System.cc

@@ -560,7 +560,7 @@ void QuestDebugScreenInit()

 	gItemListBox.usItemDisplayedOnTopOfList			= 1;
 	gItemListBox.usStartIndex										= 1;
-	gItemListBox.usMaxArrayIndex								= MAXITEMS;
+	gItemListBox.usMaxArrayIndex								= MAXITEMS.inner();


Do you really want MAXITEMS to be an ItemId? size_t looks like a better choice to me.

momoko-h · 2022-11-16T11:26:58Z

src/game/Tactical/LoadSaveObjectType.cc

@@ -38,7 +38,7 @@ void ExtractObject(DataReader& d, OBJECTTYPE* const o)
 			EXTR_U8(d, o->ubGunAmmoType)
 			EXTR_U8(d, o->ubGunShotsLeft)
 			EXTR_SKIP(d, 1)
-			EXTR_U16(d, o->usGunAmmoItem)
+			o->usGunAmmoItem = d.read<ItemId>();


You're doing this often enough that adding EXTR_ItemId and INJ_ItemId macros are probably worthwhile

momoko-h · 2022-11-16T11:31:54Z

src/game/Tactical/World_Items.cc

@@ -259,7 +259,7 @@ void LoadWorldItemsFromMap(HWFILE const f)
 	{
 		// Add all of the items to the world indirectly through AddItemToPool, but
 		// only if the chance associated with them succeed.
-		WORLDITEM wi;
+		WORLDITEM wi = {};


The very next line overwrites this initialization.

momoko-h · 2022-11-16T11:37:02Z

src/sgp/Types.h

+// as the type Inner an can be explicitly converted from / to it.
+template <typename For, typename Inner> struct TypedId {
+public:
+  constexpr TypedId() : m_id(0) {}


If you add something like using TID = TypedId<For, Inner>; here you don't have to repeat it all the time.

momoko-h · 2022-11-16T11:38:30Z

src/sgp/Types.h

+	return m_id != rhs.m_id;
+  }
+
+  TypedId<For, Inner> operator++(int i) {


Drop the i after int.

momoko-h · 2022-11-16T11:39:13Z

src/sgp/Types.h

+  }
+
+  TypedId<For, Inner> operator++(int i) {
+	auto copy = TypedId<For, Inner>(this->m_id);


Or just auto copy = *this ?

magras · 2023-08-08T09:01:56Z

An enum class implies that there is a known number of known variants.

I know I am just a passing stranger, but that's not true. There are a bunch of counterexamples from the standard:

std::byte:
```
enum class byte : unsigned char {};
```
std::align_val_t:
```
enum class align_val_t : size_t {};
```

Handle example:

enum class Handle : uint32_t { Invalid = 0 };

The last one demonstrates that the committee sees scoped enums as opaque type aliases for integral types with associated constants.

selaux · 2023-08-08T12:38:35Z

Interesting. I might have another look at that then.

Use type-safe id for items

679dd4a

momoko-h reviewed Nov 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: WIP: Use type-safe ids #1707

Proposal: WIP: Use type-safe ids #1707

selaux commented Nov 15, 2022 •

edited

lynxlynxlynx commented Nov 15, 2022

momoko-h commented Nov 15, 2022

selaux commented Nov 15, 2022 •

edited

lynxlynxlynx commented Nov 15, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

momoko-h Nov 16, 2022

magras commented Aug 8, 2023

selaux commented Aug 8, 2023

Proposal: WIP: Use type-safe ids #1707

Are you sure you want to change the base?

Proposal: WIP: Use type-safe ids #1707

Conversation

selaux commented Nov 15, 2022 • edited

lynxlynxlynx commented Nov 15, 2022

momoko-h commented Nov 15, 2022

selaux commented Nov 15, 2022 • edited

lynxlynxlynx commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magras commented Aug 8, 2023

selaux commented Aug 8, 2023

selaux commented Nov 15, 2022 •

edited

selaux commented Nov 15, 2022 •

edited