Merge branch 'rework_fixed_string' into 'master'

Rework fixed string

See merge request OpenMW/openmw!1596

apps/essimporter/importsplm.h

@@ -41,7 +41,7 @@ struct SPLM
     {
         int mUnknown;
         unsigned char mUnknown2;
-        ESM::FIXED_STRING<35> mItemId; // disintegrated item / bound item / item to re-equip after expiration
+        ESM::FixedString<35> mItemId; // disintegrated item / bound item / item to re-equip after expiration
     };
 
     struct CNAM // 36 bytes

apps/opencs/model/world/refidadapterimp.hpp

@@ -1913,14 +1913,20 @@ namespace CSMWorld
                     break; // always save
                 case 13: // NAME32
                     if (content.mType == ESM::AI_Activate)
-                        content.mActivate.mName.assign(value.toString().toUtf8().constData());
+                    {
+                        const QByteArray name = value.toString().toUtf8();
+                        content.mActivate.mName.assign(std::string_view(name.constData(), name.size()));
+                    }
                     else
                         return; // return without saving
 
                     break; // always save
                 case 14: // NAME32
                     if (content.mType == ESM::AI_Follow || content.mType == ESM::AI_Escort)
-                        content.mTarget.mId.assign(value.toString().toUtf8().constData());
+                    {
+                        const QByteArray id = value.toString().toUtf8();
+                        content.mTarget.mId.assign(std::string_view(id.constData(), id.size()));
+                    }
                     else
                         return; // return without saving
 

apps/openmw/mwmechanics/aiactivate.cpp

@@ -13,7 +13,7 @@
 
 namespace MWMechanics
 {
-    AiActivate::AiActivate(const std::string &objectId, bool repeat)
+    AiActivate::AiActivate(std::string_view objectId, bool repeat)
         : TypedAiPackage<AiActivate>(repeat), mObjectId(objectId)
     {
     }

apps/openmw/mwmechanics/aiactivate.hpp

@@ -4,6 +4,7 @@
 #include "typedaipackage.hpp"
 
 #include <string>
+#include <string_view>
 
 #include "pathfinding.hpp"
 
@@ -24,7 +25,7 @@ namespace MWMechanics
         public:
             /// Constructor
             /** \param objectId Reference to object to activate **/
-            explicit AiActivate(const std::string &objectId, bool repeat);
+            explicit AiActivate(std::string_view objectId, bool repeat);
 
             explicit AiActivate(const ESM::AiSequence::AiActivate* activate);
 

apps/openmw/mwmechanics/aiescort.cpp

@@ -20,20 +20,20 @@
 
 namespace MWMechanics
 {
-    AiEscort::AiEscort(const std::string &actorId, int duration, float x, float y, float z, bool repeat)
+    AiEscort::AiEscort(std::string_view actorId, int duration, float x, float y, float z, bool repeat)
     : TypedAiPackage<AiEscort>(repeat), mX(x), mY(y), mZ(z), mDuration(duration), mRemainingDuration(static_cast<float>(duration))
     , mCellX(std::numeric_limits<int>::max())
     , mCellY(std::numeric_limits<int>::max())
     {
-        mTargetActorRefId = actorId;
+        mTargetActorRefId = std::string(actorId);
     }
 
-    AiEscort::AiEscort(const std::string &actorId, const std::string &cellId, int duration, float x, float y, float z, bool repeat)
+    AiEscort::AiEscort(std::string_view actorId, std::string_view cellId, int duration, float x, float y, float z, bool repeat)
     : TypedAiPackage<AiEscort>(repeat), mCellId(cellId), mX(x), mY(y), mZ(z), mDuration(duration), mRemainingDuration(static_cast<float>(duration))
     , mCellX(std::numeric_limits<int>::max())
     , mCellY(std::numeric_limits<int>::max())
     {
-        mTargetActorRefId = actorId;
+        mTargetActorRefId = std::string(actorId);
     }
 
     AiEscort::AiEscort(const ESM::AiSequence::AiEscort *escort)

apps/openmw/mwmechanics/aiescort.hpp

@@ -4,6 +4,7 @@
 #include "typedaipackage.hpp"
 
 #include <string>
+#include <string_view>
 
 namespace ESM
 {
@@ -22,11 +23,11 @@ namespace MWMechanics
             /// Implementation of AiEscort
             /** The Actor will escort the specified actor to the world position x, y, z until they reach their position, or they run out of time
                 \implement AiEscort **/
-            AiEscort(const std::string &actorId, int duration, float x, float y, float z, bool repeat);
+            AiEscort(std::string_view actorId, int duration, float x, float y, float z, bool repeat);
             /// Implementation of AiEscortCell
             /** The Actor will escort the specified actor to the cell position x, y, z until they reach their position, or they run out of time
                 \implement AiEscortCell **/
-            AiEscort(const std::string &actorId, const std::string &cellId, int duration, float x, float y, float z, bool repeat);
+            AiEscort(std::string_view actorId, std::string_view cellId, int duration, float x, float y, float z, bool repeat);
 
             AiEscort(const ESM::AiSequence::AiEscort* escort);
 

apps/openmw/mwmechanics/aifollow.cpp

@@ -28,18 +28,18 @@ namespace MWMechanics
 {
 int AiFollow::mFollowIndexCounter = 0;
 
-AiFollow::AiFollow(const std::string &actorId, float duration, float x, float y, float z, bool repeat)
+AiFollow::AiFollow(std::string_view actorId, float duration, float x, float y, float z, bool repeat)
 : TypedAiPackage<AiFollow>(repeat), mAlwaysFollow(false), mDuration(duration), mRemainingDuration(duration), mX(x), mY(y), mZ(z)
 , mCellId(""), mActive(false), mFollowIndex(mFollowIndexCounter++)
 {
-    mTargetActorRefId = actorId;
+    mTargetActorRefId = std::string(actorId);
 }
 
-AiFollow::AiFollow(const std::string &actorId, const std::string &cellId, float duration, float x, float y, float z, bool repeat)
+AiFollow::AiFollow(std::string_view actorId, std::string_view cellId, float duration, float x, float y, float z, bool repeat)
 : TypedAiPackage<AiFollow>(repeat), mAlwaysFollow(false), mDuration(duration), mRemainingDuration(duration), mX(x), mY(y), mZ(z)
 , mCellId(cellId), mActive(false), mFollowIndex(mFollowIndexCounter++)
 {
-    mTargetActorRefId = actorId;
+    mTargetActorRefId = std::string(actorId);
 }
 
 AiFollow::AiFollow(const MWWorld::Ptr& actor, bool commanded)

apps/openmw/mwmechanics/aifollow.hpp

@@ -4,6 +4,7 @@
 #include "typedaipackage.hpp"
 
 #include <string>
+#include <string_view>
 
 #include <components/esm/defs.hpp>
 
@@ -38,9 +39,9 @@ namespace MWMechanics
     {
         public:
             /// Follow Actor for duration or until you arrive at a world position
-            AiFollow(const std::string &actorId, float duration, float x, float y, float z, bool repeat);
+            AiFollow(std::string_view actorId, float duration, float x, float y, float z, bool repeat);
             /// Follow Actor for duration or until you arrive at a position in a cell
-            AiFollow(const std::string &actorId, const std::string &CellId, float duration, float x, float y, float z, bool repeat);
+            AiFollow(std::string_view actorId, std::string_view cellId, float duration, float x, float y, float z, bool repeat);
             /// Follow Actor indefinitively
             AiFollow(const MWWorld::Ptr& actor, bool commanded=false);
 

apps/openmw/mwmechanics/aisequence.cpp

@@ -428,7 +428,7 @@ void AiSequence::fill(const ESM::AIPackageList &list)
         else if (esmPackage.mType == ESM::AI_Escort)
         {
             ESM::AITarget data = esmPackage.mTarget;
-            package = std::make_unique<MWMechanics::AiEscort>(data.mId.toString(), data.mDuration, data.mX, data.mY, data.mZ, data.mShouldRepeat != 0);
+            package = std::make_unique<MWMechanics::AiEscort>(data.mId.toStringView(), data.mDuration, data.mX, data.mY, data.mZ, data.mShouldRepeat != 0);
         }
         else if (esmPackage.mType == ESM::AI_Travel)
         {
@@ -438,12 +438,12 @@ void AiSequence::fill(const ESM::AIPackageList &list)
         else if (esmPackage.mType == ESM::AI_Activate)
         {
             ESM::AIActivate data = esmPackage.mActivate;
-            package = std::make_unique<MWMechanics::AiActivate>(data.mName.toString(), data.mShouldRepeat != 0);
+            package = std::make_unique<MWMechanics::AiActivate>(data.mName.toStringView(), data.mShouldRepeat != 0);
         }
         else //if (esmPackage.mType == ESM::AI_Follow)
         {
             ESM::AITarget data = esmPackage.mTarget;
-            package = std::make_unique<MWMechanics::AiFollow>(data.mId.toString(), data.mDuration, data.mX, data.mY, data.mZ, data.mShouldRepeat != 0);
+            package = std::make_unique<MWMechanics::AiFollow>(data.mId.toStringView(), data.mDuration, data.mX, data.mY, data.mZ, data.mShouldRepeat != 0);
         }
         mPackages.push_back(std::move(package));
     }

apps/openmw/mwstate/statemanagerimp.cpp

@@ -494,7 +494,7 @@ void MWState::StateManager::loadGame (const Character *character, const std::str
                 default:
 
                     // ignore invalid records
-                    Log(Debug::Warning) << "Warning: Ignoring unknown record: " << n.toString();
+                    Log(Debug::Warning) << "Warning: Ignoring unknown record: " << n.toStringView();
                     reader.skipRecord();
             }
             int progressPercent = static_cast<int>(float(reader.getFileOffset())/total*100);

apps/openmw_test_suite/esm/test_fixed_string.cpp

@@ -105,3 +105,46 @@ TEST(EsmFixedString, is_pod)
      ASSERT_TRUE(std::is_pod<ESM::NAME32>::value);
      ASSERT_TRUE(std::is_pod<ESM::NAME64>::value);
 }
+
+TEST(EsmFixedString, assign_should_zero_untouched_bytes_for_4)
+{
+    ESM::NAME value;
+    value = static_cast<uint32_t>(0xFFFFFFFFu);
+    value.assign(std::string(1, 'a'));
+    EXPECT_EQ(value, static_cast<uint32_t>('a')) << value.toInt();
+}
+
+TEST(EsmFixedString, assign_should_only_truncate_for_4)
+{
+    ESM::NAME value;
+    value.assign(std::string(5, 'a'));
+    EXPECT_EQ(value, std::string(4, 'a'));
+}
+
+TEST(EsmFixedString, assign_should_truncate_and_set_last_element_to_zero)
+{
+    ESM::FixedString<17> value;
+    value.assign(std::string(20, 'a'));
+    EXPECT_EQ(value, std::string(16, 'a'));
+}
+
+TEST(EsmFixedString, assign_should_truncate_and_set_last_element_to_zero_for_32)
+{
+    ESM::NAME32 value;
+    value.assign(std::string(33, 'a'));
+    EXPECT_EQ(value, std::string(31, 'a'));
+}
+
+TEST(EsmFixedString, assign_should_truncate_and_set_last_element_to_zero_for_64)
+{
+    ESM::NAME64 value;
+    value.assign(std::string(65, 'a'));
+    EXPECT_EQ(value, std::string(63, 'a'));
+}
+
+TEST(EsmFixedString, assignment_operator_is_supported_for_uint32)
+{
+    ESM::NAME value;
+    value = static_cast<uint32_t>(0xFEDCBA98u);
+    EXPECT_EQ(value, static_cast<uint32_t>(0xFEDCBA98u)) << value.toInt();
+}

components/esm/esmcommon.hpp

@@ -1,12 +1,11 @@
 #ifndef OPENMW_ESM_COMMON_H
 #define OPENMW_ESM_COMMON_H
 
-#include <algorithm>
 #include <string>
 #include <cstring>
 #include <vector>
-
+#include <string_view>
-#include <stdint.h>
+#include <cstdint>
 
 namespace ESM
 {
@@ -22,120 +21,110 @@ enum RecordFlag
     FLAG_Blocked    = 0x00002000
   };
 
-// CRTP for FIXED_STRING class, a structure used for holding fixed-length strings
+template <std::size_t capacity>
-template< template<size_t> class DERIVED, size_t SIZE>
+struct FixedString
-class FIXED_STRING_BASE
 {
-    /* The following methods must be implemented in derived classes:
+    static_assert(capacity > 0);
-     *   char const* ro_data() const; // return pointer to ro buffer
-     *   char*       rw_data();       // return pointer to rw buffer
-     */
-public:
-    enum { size = SIZE };
 
-    template<size_t OTHER_SIZE>
+    static constexpr std::size_t sCapacity = capacity;
-    bool operator==(char const (&str)[OTHER_SIZE]) const
+
+    char mData[capacity];
+
+    std::string_view toStringView() const noexcept
     {
-        size_t other_len = strnlen(str, OTHER_SIZE);
+        return std::string_view(mData, strnlen(mData, capacity));
-        if (other_len != this->length())
-            return false;
-        return std::strncmp(self()->ro_data(), str, size) == 0;
     }
 
-    //this operator will not be used for char[N], only for char*
+    std::string toString() const
-    template<typename T, typename = typename std::enable_if<std::is_same<T, char>::value>::type>
-    bool operator==(const T* const& str) const
     {
-        char const* const data = self()->ro_data();
+        return std::string(toStringView());
-        for(size_t i = 0; i < size; ++i)
-        {
-            if(data[i] != str[i]) return false;
-            else if(data[i] == '\0') return true;
-        }
-        return str[size] == '\0';
-    }
-    bool operator!=(const char* const str) const { return !( (*this) == str ); }
-
-    bool operator==(const std::string& str) const
-    {
-        return (*this) == str.c_str();
-    }
-    bool operator!=(const std::string& str) const { return !( (*this) == str ); }
-
-    static size_t data_size() { return size; }
-    size_t length() const { return strnlen(self()->ro_data(), size); }
-    std::string toString() const { return std::string(self()->ro_data(), this->length()); }
-
-    void assign(const std::string& value)
-    {
-        std::strncpy(self()->rw_data(), value.c_str(), size-1);
-        self()->rw_data()[size-1] = '\0';
     }
 
-    void clear() { this->assign(""); }
+    std::uint32_t toInt() const noexcept
-private:
-    DERIVED<size> const* self() const
-    {
-        return static_cast<DERIVED<size> const*>(this);
-    }
-
-    // write the non-const version in terms of the const version
-    // Effective C++ 3rd ed., Item 3 (p. 24-25)
-    DERIVED<size>* self()
-    {
-        return const_cast<DERIVED<size>*>(static_cast<FIXED_STRING_BASE const*>(this)->self());
-    }
-};
-
-// Generic implementation
-template <size_t SIZE>
-struct FIXED_STRING : public FIXED_STRING_BASE<FIXED_STRING, SIZE>
-{
-    char data[SIZE];
-
-    char const* ro_data() const { return data; }
-    char*       rw_data() { return data; }
-};
-
-// In the case of SIZE=4, it can be more efficient to match the string
-// as a 32 bit number, therefore the struct is implemented as a union with an int.
-template <>
-struct FIXED_STRING<4> : public FIXED_STRING_BASE<FIXED_STRING, 4>
-{
-    char data[4];
-
-    using FIXED_STRING_BASE::operator==;
-    using FIXED_STRING_BASE::operator!=;
-
-    bool operator==(uint32_t v) const { return v == toInt(); }
-    bool operator!=(uint32_t v) const { return v != toInt(); }
-
-    FIXED_STRING<4>& operator=(std::uint32_t value)
-    {
-        std::memcpy(data, &value, sizeof(data));
-        return *this;
-    }
-
-    void assign(const std::string& value)
-    {
-        std::memset(data, 0, sizeof(data));
-        std::memcpy(data, value.data(), std::min(value.size(), sizeof(data)));
-    }
-
-    char const* ro_data() const { return data; }
-    char*       rw_data() { return data; }
-
-    std::uint32_t toInt() const
     {
+        static_assert(capacity == sizeof(std::uint32_t));
         std::uint32_t value;
-        std::memcpy(&value, data, sizeof(data));
+        std::memcpy(&value, mData, capacity);
         return value;
     }
+
+    void clear() noexcept
+    {
+        std::memset(mData, 0, capacity);
+    }
+
+    void assign(std::string_view value) noexcept
+    {
+        if (value.empty())
+        {
+            clear();
+            return;
+        }
+
+        if (value.size() < capacity)
+        {
+            if constexpr (capacity == sizeof(std::uint32_t))
+                std::memset(mData, 0, capacity);
+            std::memcpy(mData, value.data(), value.size());
+            if constexpr (capacity != sizeof(std::uint32_t))
+                mData[value.size()] = '\0';
+            return;
+        }
+
+        std::memcpy(mData, value.data(), capacity);
+
+        if constexpr (capacity != sizeof(std::uint32_t))
+            mData[capacity - 1] = '\0';
+    }
+
+    FixedString& operator=(std::uint32_t value) noexcept
+    {
+        static_assert(capacity == sizeof(value));
+        std::memcpy(&mData, &value, capacity);
+        return *this;
+    }
 };
 
-typedef FIXED_STRING<4> NAME;
+template <std::size_t capacity, class T, typename = std::enable_if_t<std::is_same_v<T, char>>>
-typedef FIXED_STRING<32> NAME32;
+inline bool operator==(const FixedString<capacity>& lhs, const T* const& rhs) noexcept
-typedef FIXED_STRING<64> NAME64;
+{
+    for (std::size_t i = 0; i < capacity; ++i)
+    {
+        if (lhs.mData[i] != rhs[i])
+            return false;
+        if (lhs.mData[i] == '\0')
+            return true;
+    }
+    return rhs[capacity] == '\0';
+}
+
+template <std::size_t capacity>
+inline bool operator==(const FixedString<capacity>& lhs, const std::string& rhs) noexcept
+{
+    return lhs == rhs.c_str();
+}
+
+template <std::size_t capacity, std::size_t rhsSize>
+inline bool operator==(const FixedString<capacity>& lhs, const char (&rhs)[rhsSize]) noexcept
+{
+    return strnlen(rhs, rhsSize) == strnlen(lhs.mData, capacity)
+        && std::strncmp(lhs.mData, rhs, capacity) == 0;
+}
+
+inline bool operator==(const FixedString<4>& lhs, std::uint32_t rhs) noexcept
+{
+    return lhs.toInt() == rhs;
+}
+
+template <std::size_t capacity, class Rhs>
+inline bool operator!=(const FixedString<capacity>& lhs, const Rhs& rhs) noexcept
+{
+    return !(lhs == rhs);
+}
+
+using NAME = FixedString<4>;
+using NAME32 = FixedString<32>;
+using NAME64 = FixedString<64>;
 
 /* This struct defines a file 'context' which can be saved and later
    restored by an ESMReader instance. It will save the position within

components/esm3/esmreader.cpp

@@ -208,9 +208,9 @@ void ESMReader::getSubName()
     }
 
     // reading the subrecord data anyway.
-    const int subNameSize = static_cast<int>(mCtx.subName.data_size());
+    const std::size_t subNameSize = decltype(mCtx.subName)::sCapacity;
-    getExact(mCtx.subName.rw_data(), subNameSize);
+    getExact(mCtx.subName.mData, static_cast<int>(subNameSize));
-    mCtx.leftRec -= static_cast<uint32_t>(subNameSize);
+    mCtx.leftRec -= static_cast<std::uint32_t>(subNameSize);
 }
 
 void ESMReader::skipHSub()
@@ -257,7 +257,7 @@ NAME ESMReader::getRecName()
     if (!hasMoreRecs())
         fail("No more records, getRecName() failed");
     getName(mCtx.recName);
-    mCtx.leftFile -= mCtx.recName.data_size();
+    mCtx.leftFile -= decltype(mCtx.recName)::sCapacity;
 
     // Make sure we don't carry over any old cached subrecord
     // names. This can happen in some cases when we skip parts of a
@@ -331,8 +331,8 @@ std::string ESMReader::getString(int size)
 
     ss << "ESM Error: " << msg;
     ss << "\n  File: " << mCtx.filename;
-    ss << "\n  Record: " << mCtx.recName.toString();
+    ss << "\n  Record: " << mCtx.recName.toStringView();
-    ss << "\n  Subrecord: " << mCtx.subName.toString();
+    ss << "\n  Subrecord: " << mCtx.subName.toStringView();
     if (mEsm.get())
         ss << "\n  Offset: 0x" << std::hex << mEsm->tellg();
     throw std::runtime_error(ss.str());

components/esm3/loadscpt.cpp

@@ -47,7 +47,7 @@ namespace ESM
             std::stringstream ss;
             ss << "Malformed string table";
             ss << "\n  File: " << esm.getName();
-            ss << "\n  Record: " << esm.getContext().recName.toString();
+            ss << "\n  Record: " << esm.getContext().recName.toStringView();
             ss << "\n  Subrecord: " << "SCVR";
             ss << "\n  Offset: 0x" << std::hex << esm.getFileOffset();
             Log(Debug::Verbose) << ss.str();
@@ -68,7 +68,7 @@ namespace ESM
                     std::stringstream ss;
                     ss << "String table overflow";
                     ss << "\n  File: " << esm.getName();
-                    ss << "\n  Record: " << esm.getContext().recName.toString();
+                    ss << "\n  Record: " << esm.getContext().recName.toStringView();
                     ss << "\n  Subrecord: " << "SCVR";
                     ss << "\n  Offset: 0x" << std::hex << esm.getFileOffset();
                     Log(Debug::Verbose) << ss.str();

components/esm3/stolenitems.cpp

@@ -32,15 +32,14 @@ namespace ESM
             std::map<std::pair<std::string, bool>, int> ownerMap;
             while (esm.isNextSub("FNAM") || esm.isNextSub("ONAM"))
             {
-                std::string subname = esm.retSubName().toString();
+                const bool isFaction = (esm.retSubName() == "FNAM");
                 std::string owner = esm.getHString();
-                bool isFaction = (subname == "FNAM");
                 int count;
                 esm.getHNT(count, "COUN");
-                ownerMap.insert(std::make_pair(std::make_pair(owner, isFaction), count));
+                ownerMap.emplace(std::make_pair(std::move(owner), isFaction), count);
             }
 
-            mStolenItems[itemid] = ownerMap;
+            mStolenItems.insert_or_assign(std::move(itemid), std::move(ownerMap));
         }
     }