Rename `CString`->`String` and Update Implementation#202

Open

Mozz3d wants to merge 14 commits intowopss:masterfrom

Mozz3d:string-update

Contributor

Mozz3d commented Mar 4, 2026

No description provided.

Mozz3d added 10 commits

March 3, 2026 22:04


          Rename CString->String and Update Implementation

764a622


          Fix incorrect forward decl in StringView.hpp

a041412

And outdated use of `CString` in the `function_registration/Main.cpp` example file


          Fix issues

eb6d39d

- Fix some more incorrect forward decls
- Remove a mostly unnecessary function that led to a circular dependency (something to resolve later)
- Manual casts to avoid warnings


          Formatting

73043c9


          Formatting

75742c7


          FORMATTING


          Improve StringView & consolidate String logic through it

aa6adea

Also moved impls to inl file to avoid circular dependency


          Move constexpr methods in StringView inline to resolve static lib l…

610f41e

…inker errors


          Formatting

02832bc

idk why pre commit isnt working


          Formatting

f102d67

wopss requested changes

View reviewed changes

Owner

wopss left a comment

I took a quick look at the class API, will do more in-depth review later.

include/RED4ext/String.hpp Outdated

+                  String(const std::string& aStr, const Memory::IAllocator* aAllocator = nullptr);
+                  ~String();
+                  operator std::string() const;

Owner

wopss Mar 9, 2026

Suggested change

      
                operator std::string() const;
          
                [[nodiscard]] operator std::string() const;

include/RED4ext/String.hpp Outdated

Comment on lines +49 to +51

		operator std::string() const;

		[[nodiscard]] operator std::string_view() const noexcept;

Owner

wopss Mar 9, 2026

I would put the conversion operators after the operator= or even at the end of all operators, but that's my preference, you can ignore it.

include/RED4ext/String.hpp Outdated

+                  String(const String& aOther);
+                  String(String&& aOther) noexcept;
+                  String(const std::string& aStr, const Memory::IAllocator* aAllocator = nullptr);
+                  ~String();

Owner

wopss Mar 9, 2026

Suggested change

      
                ~String();
          
                ~String();

include/RED4ext/String.hpp Outdated

+                  Reference operator[](SizeType aIndex) noexcept;
+                  ConstReference operator[](SizeType aIndex) const noexcept;
+                  String& Assign(ConstPointer aStr, SizeType aSize);

Owner

wopss Mar 9, 2026

I would remove this, since StringView does the job here.

include/RED4ext/String.hpp Outdated


		operator std::string() const;

		[[nodiscard]] operator std::string_view() const noexcept;

Owner

wopss Mar 9, 2026

I don't remember if we spoke about this, but should [[nodiscard]] be on the same line or new line?

Collaborator

psiberx Mar 10, 2026

I don't remember if we ever discussed it.

Owner

wopss Mar 10, 2026

What is your preference in this case? I would prefer it on a new line.

include/RED4ext/StringView.hpp

+                  {
+                  }
+                  StringView(const String& aStr) noexcept;

Owner

wopss Mar 9, 2026

You are missing the move operations.

include/RED4ext/StringView.hpp

+                  {
+                  }
+                  StringView(const String& aStr) noexcept;

Owner

wopss Mar 9, 2026

This can also be constexpr.

include/RED4ext/StringView.hpp

Comment on lines +48 to +49

		bool operator==(const String& aRhs) const noexcept;
		bool operator!=(const String& aRhs) const noexcept;

Owner

wopss Mar 9, 2026

Suggested change

      
                bool operator==(const String& aRhs) const noexcept;
          
                bool operator!=(const String& aRhs) const noexcept;
          
                constexpr bool operator==(const String& aRhs) const noexcept;
          
                constexpr bool operator!=(const String& aRhs) const noexcept;

include/RED4ext/StringView.hpp

Comment on lines +81 to +84

+                  constexpr operator bool() const noexcept
+                  {
+                      return !IsEmpty();
+                  }

Owner

wopss Mar 9, 2026

Same comment as above about implicit conversions.

include/RED4ext/StringView.hpp

+                  [[nodiscard]] constexpr bool Compare(StringView aOther) const noexcept
+                  {
+                      if (IsEmpty() && aOther.IsEmpty())
+                          return true;

Owner

wopss Mar 9, 2026

Brackets!

Contributor Author

Mozz3d Mar 9, 2026

What exactly is the rule on brackets? Evidently, I'm not clear on it

Owner

wopss Mar 10, 2026

Always use brackets.

Contributor

poirierlouis Mar 10, 2026

Better for maintenance as scope is ready to add more statements, some logs when debugging, easier to read, etc.

psiberx requested changes

View reviewed changes

include/RED4ext/String.hpp Outdated

+                  String(ConstPointer aStr, const Memory::IAllocator* aAllocator = nullptr);
+                  String(const String& aOther);
+                  String(String&& aOther) noexcept;
+                  String(const std::string& aStr, const Memory::IAllocator* aAllocator = nullptr);

Collaborator

psiberx Mar 10, 2026

Should have one for string_view too.

include/RED4ext/String.hpp Outdated

Comment on lines +49 to +51

		operator std::string() const;

		[[nodiscard]] operator std::string_view() const noexcept;

Collaborator

psiberx Mar 10, 2026

Generally yes, I consider implicit conversions unsafe in the first place. But I can't think of any good examples for this case.

include/RED4ext/String.hpp Outdated

Comment on lines +131 to +134

+                  [[nodiscard]] bool IsBackInternal() const noexcept;
+                  [[nodiscard]] bool IsBackAllocated() const noexcept;
+                  [[nodiscard]] bool IsBackViewing() const noexcept;
+                  [[nodiscard]] bool IsBackExternal() const noexcept;

Collaborator

psiberx Mar 10, 2026

I agree, these methods should be private (for internal use) of removed.

include/RED4ext/String.hpp Outdated

+              #pragma endregion
+                  union Storage
+                  {
+                      enum class BackType : uint32_t

Collaborator

psiberx Mar 10, 2026

Perhaps StorageMode is a better name.

include/RED4ext/String.hpp Outdated

Comment on lines +181 to +183

+                          Internal = 0,  // owned, inline
+                          Allocated = 1, // owned, external
+                          Viewing = 2    // unowned, external

Collaborator

psiberx Mar 10, 2026

We can continue to use Inline, same term is used for Variant class.
For other two I can suggest DynamicallyAllocated and ExternallyAllocated.

include/RED4ext/String.hpp Outdated

+                      {
+                          Internal = 0,  // owned, inline
+                          Allocated = 1, // owned, external
+                          Viewing = 2    // unowned, external

Collaborator

psiberx Mar 10, 2026

BTW, how to create externally allocated String right now?

Contributor Author

Mozz3d Mar 10, 2026

It's all done in SetCapacity() (1744572317)
It also handles the transition from inline to allocated and vice versa

Collaborator

psiberx Mar 10, 2026 •

edited

Loading

I don't get it. How it can transfer ownership in SetCapacity? This doesn't make sense.

Collaborator

psiberx Mar 10, 2026

I checked the function. It doesn't do anything if storage is set to external as expected.

Contributor Author

Mozz3d Mar 10, 2026

Wdym? Like from viewing to allocated? It doesn't

Contributor Author

Mozz3d Mar 10, 2026

Yeah, my bad, I didn't understand what you were asking.
String can go from inline -> allocated and back, but it can't take ownership if viewing

Owner

wopss Mar 10, 2026

So when you said `owned, external you meant "allocated on heap, managed by this class"?

Contributor Author

Mozz3d Mar 10, 2026

Yes

wopss reviewed

View reviewed changes

include/RED4ext/String.hpp Outdated

+                      {
+                          Internal = 0,  // owned, inline
+                          Allocated = 1, // owned, external
+                          Viewing = 2    // unowned, external

Owner

wopss Mar 10, 2026

So when you said `owned, external you meant "allocated on heap, managed by this class"?

include/RED4ext/String.hpp Outdated

+                      {
+                          Internal = 0,  // owned, inline
+                          Allocated = 1, // owned, external
+                          Viewing = 2    // unowned, external

Owner

wopss Mar 10, 2026

What is viewing? How can a string have an unowned string?

Contributor Author

Mozz3d Mar 10, 2026

When string is in that mode it's pretty much no different in functionality than StringView
String manages a ptr and size for the data, but does not own anything about the data

Owner

wopss Mar 10, 2026

That's really odd, is this even used in game? Maybe they share this with the StringView?

Contributor Author

Mozz3d Mar 13, 2026

I looked into this much more in-depth, and it turns out my initial assumption wasn't necessarily right. This mode is used in the binary but only ever with stack-allocated scratch buffers. So it's more accurately a "scratch mode."
While the String still doesn't own the memory, it looks like it's always used as an in-place "API" over a piece of the stack for building.
I personally don't like this; it's dangerous and awkward, but it is what it is.

Contributor Author

Mozz3d Mar 13, 2026

Such a use case really should be a dedicated type. IDK why they decided to cram it into the default String, but it's probably because they didn't have time to rebuild the string API

How do we want to do this?

Owner

wopss Mar 17, 2026

Would it be possible that they have a different class for this? Something like a nonmovable and noncopyable string.

Also, is there a pattern to that? Maybe they have more small string optimizations.

How do we want to do this?

We should just mention it here, but never allow for it to be used in the SDK.

Collaborator

psiberx Mar 17, 2026 •

edited

Loading

I think the most dangerous part is mutability. We could allow it as const String only with a static helper function.
As for use cases, they seem to use it for const strings for their own interfaces that require String, e.g. "NULL" for RTTI/serialization.

Mozz3d added 4 commits

March 14, 2026 06:08


          Merge branch 'master' into string-update

bf0b716


          Suggested changes

fb63383

String:
- Extract `String::Storage` union from within `String` and rename to `StringStorage`
- Extract `String::Storage::BackType` to standalone enum and rename to `EStringMode`
- Rename `EStringMode` fields to better reflect usage
- Fix allocator constness
- Remove uses of `ConstPointer, SizeType` methods in favor of `StringView`
- Add `std::string_view` constructor
- Removed implicit conversion operators
- Removed `CalcHash()`
- Removed unnecessary STL compliant definitions
- Removed mode check methods in favor of more explicit expressions
- Always use brackets

StringView:
- Document default move/copy constructors
- Make methods revolving `String` constexpr
- Always use brackets


          Remove constexpr from StringView methods involving String

I imagined this being an issue


          Fix formatting

6f5a00d

I figured out pre-commit hook!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet