Change FormId::toString to be consistent with RefId. Remove FormIdRefId.

apps/opencs/model/world/columnimp.cpp

@@ -22,6 +22,8 @@ namespace CSMWorld
 
             std::string operator()(ESM::StringRefId value) const { return value.getValue(); }
 
+            std::string operator()(ESM::FormId value) const { return value.toString("FormId:"); }
+
             std::string operator()(ESM::IndexRefId value) const
             {
                 switch (value.getRecordType())

apps/openmw/mwlua/luabindings.cpp

@@ -268,9 +268,9 @@ namespace MWLua
         };
         api["getObjectByFormId"] = [](std::string_view formIdStr) -> GObject {
             ESM::RefId refId = ESM::RefId::deserializeText(formIdStr);
-            if (!refId.is<ESM::FormIdRefId>())
+            if (!refId.is<ESM::FormId>())
                 throw std::runtime_error("FormId expected, got " + std::string(formIdStr) + "; use core.getFormId");
-            return GObject(refId.getIf<ESM::FormIdRefId>()->getValue());
+            return GObject(*refId.getIf<ESM::FormId>());
         };
 
         // Creates a new record in the world database.

apps/openmw/mwlua/nearbybindings.cpp

@@ -126,9 +126,9 @@ namespace MWLua
 
         api["getObjectByFormId"] = [](std::string_view formIdStr) -> LObject {
             ESM::RefId refId = ESM::RefId::deserializeText(formIdStr);
-            if (!refId.is<ESM::FormIdRefId>())
+            if (!refId.is<ESM::FormId>())
                 throw std::runtime_error("FormId expected, got " + std::string(formIdStr) + "; use core.getFormId");
-            return LObject(refId.getIf<ESM::FormIdRefId>()->getValue());
+            return LObject(*refId.getIf<ESM::FormId>());
         };
 
         api["activators"] = LObjectList{ objectLists->getActivatorsInScene() };

apps/openmw_test_suite/esm/testrefid.cpp

@@ -9,6 +9,8 @@
 #include <map>
 #include <string>
 
+#include "../testing_util.hpp"
+
 MATCHER(IsPrint, "")
 {
     return std::isprint(arg) != 0;
@@ -38,6 +40,12 @@ namespace ESM
             EXPECT_FALSE(refId.empty());
         }
 
+        TEST(ESMRefIdTest, FormIdRefIdMustHaveContentFile)
+        {
+            EXPECT_TRUE(RefId(FormId()).empty());
+            EXPECT_ERROR(RefId(FormId{ .mIndex = 1, .mContentFile = -1 }), "RefId can't be a generated FormId");
+        }
+
         TEST(ESMRefIdTest, defaultConstructedIsEqualToItself)
         {
             const RefId refId;
@@ -104,11 +112,10 @@ namespace ESM
             EXPECT_EQ(stringRefId, refId);
         }
 
-        TEST(ESMRefIdTest, equalityIsDefinedForFormRefIdAndRefId)
+        TEST(ESMRefIdTest, equalityIsDefinedForFormIdAndRefId)
         {
-            const FormIdRefId formIdRefId({ .mIndex = 42, .mContentFile = 0 });
+            const FormId formId{ .mIndex = 42, .mContentFile = 0 };
-            const RefId refId = RefId::formIdRefId({ .mIndex = 42, .mContentFile = 0 });
+            EXPECT_EQ(formId, RefId(formId));
-            EXPECT_EQ(formIdRefId, refId);
         }
 
         TEST(ESMRefIdTest, stringRefIdIsEqualToItself)
@@ -139,9 +146,9 @@ namespace ESM
 
         TEST(ESMRefIdTest, lessThanIsDefinedForFormRefIdAndRefId)
         {
-            const FormIdRefId formIdRefId({ .mIndex = 13, .mContentFile = 0 });
+            const FormId formId{ .mIndex = 13, .mContentFile = 0 };
-            const RefId refId = RefId::formIdRefId({ .mIndex = 42, .mContentFile = 0 });
+            const RefId refId = RefId(FormId{ .mIndex = 42, .mContentFile = 0 });
-            EXPECT_LT(formIdRefId, refId);
+            EXPECT_LT(formId, refId);
         }
 
         TEST(ESMRefIdTest, stringRefIdHasCaseInsensitiveHash)
@@ -191,11 +198,6 @@ namespace ESM
             EXPECT_FALSE(formIdRefId < stringView);
         }
 
-        TEST(ESMRefIdTest, formIdRefIdIndexShouldHaveOnly24SignificantBits)
-        {
-            EXPECT_THROW(FormIdRefId(FormId{ .mIndex = 1 << 25, .mContentFile = 0 }), std::invalid_argument);
-        }
-
         TEST(ESMRefIdTest, canBeUsedAsMapKeyWithLookupByStringView)
         {
             const std::map<RefId, int, std::less<>> map({ { RefId::stringRefId("a"), 42 } });
@@ -256,8 +258,6 @@ namespace ESM
             { RefId::stringRefId("foo"), "foo" },
             { RefId::stringRefId(std::string({ 'a', 0, -1, '\n', '\t' })), { 'a', 0, -1, '\n', '\t' } },
             { RefId::formIdRefId({ .mIndex = 42, .mContentFile = 0 }), "0x2a" },
-            { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = std::numeric_limits<std::int32_t>::min() }),
-                "0xff80000000ffffff" },
             { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = std::numeric_limits<std::int32_t>::max() }),
                 "0x7fffffffffffff" },
             { RefId::generated(42), "0x2a" },
@@ -302,8 +302,6 @@ namespace ESM
             { RefId::stringRefId("\xff\x9b"), "\"\\xff\\x9b\"" },
             { RefId::stringRefId("\xd0\xd0"), "\"\\xd0\\xd0\"" },
             { RefId::formIdRefId({ .mIndex = 42, .mContentFile = 0 }), "FormId:0x2a" },
-            { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = std::numeric_limits<std::int32_t>::min() }),
-                "FormId:0xff80000000ffffff" },
             { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = std::numeric_limits<std::int32_t>::max() }),
                 "FormId:0x7fffffffffffff" },
             { RefId::generated(42), "Generated:0x2a" },
@@ -344,9 +342,6 @@ namespace ESM
             { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = 0x1abc }), "FormId:0x1abcffffff" },
             { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = std::numeric_limits<std::int32_t>::max() }),
                 "FormId:0x7fffffffffffff" },
-            { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = -1 }), "FormId:0xffffffffffffffff" },
-            { RefId::formIdRefId({ .mIndex = 0xffffff, .mContentFile = std::numeric_limits<std::int32_t>::min() }),
-                "FormId:0xff80000000ffffff" },
             { RefId::generated(0), "Generated:0x0" },
             { RefId::generated(1), "Generated:0x1" },
             { RefId::generated(0x1f), "Generated:0x1f" },
@@ -390,9 +385,9 @@ namespace ESM
         };
 
         template <>
-        struct GenerateRefId<FormIdRefId>
+        struct GenerateRefId<FormId>
         {
-            static RefId call() { return RefId::formIdRefId({ .mIndex = 42, .mContentFile = 0 }); }
+            static RefId call() { return FormId{ .mIndex = 42, .mContentFile = 0 }; }
         };
 
         template <>

components/CMakeLists.txt

@@ -120,7 +120,6 @@ add_component_dir (to_utf8
 
 add_component_dir(esm attr common defs esmcommon records util luascripts format refid esmbridge esmterrain
     formid
-    formidrefid
     stringrefid
     generatedrefid
     indexrefid

components/esm/formid.cpp

@@ -1,8 +1,33 @@
 #include "formid.hpp"
 
-std::string ESM::FormId::toString() const
+#include <charconv>
+#include <cstring>
+
+std::string ESM::FormId::toString(std::string_view prefix) const
 {
-    return std::to_string(mIndex) + "_" + std::to_string(mContentFile);
+    std::string res;
+    res.resize(prefix.length() + 20);
+    std::memcpy(res.data(), prefix.data(), prefix.size());
+    char* buf = res.data() + prefix.size();
+    uint64_t value;
+    if (hasContentFile())
+    {
+        if ((mIndex & 0xff000000) != 0)
+            throw std::invalid_argument("Invalid FormId index value: " + std::to_string(mIndex));
+        value = mIndex | (static_cast<uint64_t>(mContentFile) << 24);
+    }
+    else
+    {
+        *(buf++) = '@';
+        value = mIndex | (static_cast<uint64_t>(-mContentFile - 1) << 32);
+    }
+    *(buf++) = '0';
+    *(buf++) = 'x';
+    const auto r = std::to_chars(buf, res.data() + res.size(), value, 16);
+    if (r.ec != std::errc())
+        throw std::system_error(std::make_error_code(r.ec), "ESM::FormId::toString failed");
+    res.resize(r.ptr - res.data());
+    return res;
 }
 
 uint32_t ESM::FormId::toUint32() const

components/esm/formid.hpp

@@ -21,7 +21,7 @@ namespace ESM
         // Zero is used in ESM4 as a null reference
         constexpr bool isZeroOrUnset() const { return mIndex == 0 && (mContentFile == 0 || mContentFile == -1); }
 
-        std::string toString() const;
+        std::string toString(std::string_view prefix = "") const;
         FormId32 toUint32() const;
         static constexpr FormId fromUint32(FormId32 v) { return { v & 0xffffff, static_cast<int32_t>(v >> 24) }; }
     };

components/esm/formidrefid.cpp

@@ -1,38 +0,0 @@
-#include "formidrefid.hpp"
-
-#include <ostream>
-
-#include "serializerefid.hpp"
-
-namespace ESM
-{
-    namespace
-    {
-        std::uint64_t truncate(FormId value)
-        {
-            return (static_cast<std::uint64_t>(value.mContentFile) << 24) | value.mIndex;
-        }
-    }
-
-    std::string FormIdRefId::toString() const
-    {
-        std::string result;
-        const std::uint64_t v = truncate(mValue);
-        result.resize(getHexIntegralSizeWith0x(v), '\0');
-        serializeHexIntegral(v, 0, result);
-        return result;
-    }
-
-    std::string FormIdRefId::toDebugString() const
-    {
-        std::string result;
-        const std::uint64_t v = truncate(mValue);
-        serializeRefIdValue(v, formIdRefIdPrefix, result);
-        return result;
-    }
-
-    std::ostream& operator<<(std::ostream& stream, FormIdRefId value)
-    {
-        return stream << value.toDebugString();
-    }
-}

components/esm/formidrefid.hpp

@@ -1,52 +0,0 @@
-#ifndef OPENMW_COMPONENTS_ESM_FORMIDREFID_HPP
-#define OPENMW_COMPONENTS_ESM_FORMIDREFID_HPP
-
-#include <functional>
-#include <iosfwd>
-#include <stdexcept>
-
-#include <components/esm/formid.hpp>
-
-namespace ESM
-{
-    class FormIdRefId
-    {
-    public:
-        constexpr FormIdRefId() = default;
-
-        constexpr explicit FormIdRefId(ESM::FormId value)
-            : mValue(value)
-        {
-            if ((mValue.mIndex & 0xff000000) != 0)
-                throw std::invalid_argument("Invalid FormIdRefId index value: " + std::to_string(mValue.mIndex));
-        }
-
-        ESM::FormId getValue() const { return mValue; }
-
-        std::string toString() const;
-
-        std::string toDebugString() const;
-
-        constexpr bool operator==(FormIdRefId rhs) const noexcept { return mValue == rhs.mValue; }
-
-        constexpr bool operator<(FormIdRefId rhs) const noexcept { return mValue < rhs.mValue; }
-
-        friend std::ostream& operator<<(std::ostream& stream, FormIdRefId value);
-
-        friend struct std::hash<FormIdRefId>;
-
-    private:
-        ESM::FormId mValue;
-    };
-}
-
-namespace std
-{
-    template <>
-    struct hash<ESM::FormIdRefId>
-    {
-        std::size_t operator()(ESM::FormIdRefId value) const noexcept { return std::hash<ESM::FormId>{}(value.mValue); }
-    };
-}
-
-#endif

components/esm/refid.cpp

@@ -116,6 +116,8 @@ namespace ESM
 
             std::string operator()(ESM::StringRefId v) const { return Misc::StringUtils::lowerCase(v.getValue()); }
 
+            std::string operator()(ESM::FormId v) const { return v.toString(formIdRefIdPrefix); }
+
             template <class T>
             std::string operator()(const T& v) const
             {
@@ -124,21 +126,6 @@ namespace ESM
         };
     }
 
-    std::string EmptyRefId::toString() const
-    {
-        return std::string();
-    }
-
-    std::string EmptyRefId::toDebugString() const
-    {
-        return "Empty{}";
-    }
-
-    std::ostream& operator<<(std::ostream& stream, EmptyRefId value)
-    {
-        return stream << value.toDebugString();
-    }
-
     bool RefId::operator==(std::string_view rhs) const
     {
         return std::visit(IsEqualToString{ rhs }, mValue);
@@ -154,11 +141,6 @@ namespace ESM
         return std::visit(IsGreaterThanString{ lhs }, rhs.mValue);
     }
 
-    std::ostream& operator<<(std::ostream& stream, RefId value)
-    {
-        return std::visit([&](auto v) -> std::ostream& { return stream << v; }, value.mValue);
-    }
-
     RefId RefId::stringRefId(std::string_view value)
     {
         if (value.empty())
@@ -178,7 +160,14 @@ namespace ESM
 
     std::string RefId::toDebugString() const
     {
-        return std::visit([](auto v) { return v.toDebugString(); }, mValue);
+        return std::visit(
+            [](auto v) {
+                if constexpr (std::is_same_v<decltype(v), FormId>)
+                    return v.toString(formIdRefIdPrefix);
+                else
+                    return v.toDebugString();
+            },
+            mValue);
     }
 
     bool RefId::startsWith(std::string_view prefix) const

components/esm/refid.hpp

@@ -11,7 +11,7 @@
 #include <components/misc/notnullptr.hpp>
 
 #include "esm3exteriorcellrefid.hpp"
-#include "formidrefid.hpp"
+#include "formid.hpp"
 #include "generatedrefid.hpp"
 #include "indexrefid.hpp"
 #include "stringrefid.hpp"
@@ -24,11 +24,9 @@ namespace ESM
 
         constexpr bool operator<(EmptyRefId /*rhs*/) const { return false; }
 
-        std::string toString() const;
+        std::string toString() const { return ""; }
 
-        std::string toDebugString() const;
+        std::string toDebugString() const { return "Empty{}"; }
-
-        friend std::ostream& operator<<(std::ostream& stream, EmptyRefId value);
     };
 
     enum class RefIdType : std::uint8_t
@@ -48,8 +46,7 @@ namespace ESM
     class RefId
     {
     public:
-        using Value
+        using Value = std::variant<EmptyRefId, ESM3ExteriorCellRefId, StringRefId, FormId, GeneratedRefId, IndexRefId>;
-            = std::variant<EmptyRefId, ESM3ExteriorCellRefId, StringRefId, FormIdRefId, GeneratedRefId, IndexRefId>;
 
         // Constructs RefId from a serialized string containing byte by byte copy of RefId::mValue.
         static ESM::RefId deserialize(std::string_view value);
@@ -85,17 +82,14 @@ namespace ESM
         {
         }
 
-        constexpr RefId(FormIdRefId value)
-            : mValue(value)
-        {
-        }
-
         constexpr RefId(FormId value)
         {
             if (value.isZeroOrUnset())
                 mValue = EmptyRefId();
+            else if (value.hasContentFile())
+                mValue = value;
             else
-                mValue = FormIdRefId(value);
+                throw std::logic_error("RefId can't be a generated FormId");
         }
 
         constexpr RefId(GeneratedRefId value) noexcept
@@ -164,7 +158,7 @@ namespace ESM
 
         friend bool operator<(std::string_view lhs, RefId rhs);
 
-        friend std::ostream& operator<<(std::ostream& stream, RefId value);
+        friend std::ostream& operator<<(std::ostream& stream, RefId value) { return stream << value.toDebugString(); }
 
         template <class F, class... T>
         friend constexpr auto visit(F&& f, T&&... v)

components/esm3/esmreader.cpp

@@ -498,7 +498,7 @@ namespace ESM
                 FormId formId{};
                 getTSized<8>(formId);
                 if (applyContentFileMapping(formId))
-                    return RefId::formIdRefId(formId);
+                    return RefId(formId);
                 else
                     return RefId(); // content file was removed from load order
             }

components/esm3/esmwriter.cpp

@@ -43,10 +43,10 @@ namespace ESM
                 mWriter.write(v.getValue().data(), v.getValue().size());
             }
 
-            void operator()(FormIdRefId v) const
+            void operator()(FormId v) const
             {
                 mWriter.writeT(RefIdType::FormId);
-                mWriter.writeT(v.getValue());
+                mWriter.writeT(v);
             }
 
             void operator()(GeneratedRefId v) const