Skip to content

Upgrade db to compile with v145 by default - #5

Open
ccptoebeans wants to merge 5 commits into
carbonengine:mainfrom
ccptoebeans:v145
Open

Upgrade db to compile with v145 by default#5
ccptoebeans wants to merge 5 commits into
carbonengine:mainfrom
ccptoebeans:v145

Conversation

@ccptoebeans

@ccptoebeans ccptoebeans commented Jul 30, 2026

Copy link
Copy Markdown
Member

Configuration Changes

  • Changed windows kotlin configuration to build with MSVC v145 by default.
  • Removed dependency pin for old protobuf version (blue no longer requires the version pin)
  • Changed cmake presets to use v145 by default.
  • Updated versions of microsoft/vcpkg & carbonengine/vcpkg-registry submodules to latest

Code Changes

  • returning an int 0 from a PyObject* type function is now a compilation error, these have been replaced with nullptr returns.
  • typedefs have been replaced with using, however this was just a matter of taste, using X = Y is more modern, and easier to read (in my humble opinion 🙇) than typedef Y X
  • db was using an old stdext class hash_set this has been removed from the latest version of the MSVC toolchain, and so needed to be replaced. It's been replaced with a standard unordered_set.
  • Replacing hash_set with unordered_set required exposing the hash function for InternalStoreElement, so that it could be used in functor templates struct std::hash<WStringStoreElement>, std::hash<ByteStoreElement> & std::hash<StringStoreElement>.

Note

There seems to be a bug, both in the old & new versions of the code, where if you try to insert a >64 bytes string into the store that has an identical first 64 bytes as a different string already inside the string store, the store will treat it as an identical string, and the string store will not store both strings. This may not be a bug if DB stringStore does not support strings longer than 64 bytes. If for say the store is only responsible for column names for example🤷.

This requires more investigation, but this v145 upgrade work does not affect this bug

@CCP-Aporia

Copy link
Copy Markdown
Member

Thanks, this looks good to me!

Note

There seems to be a bug, both in the old & new versions of the code, where if you try to insert a >64 bytes string into the store that has an identical first 64 bytes as a different string already inside the string store, the store will treat it as an identical string, and the string store will not store both strings. This may not be a bug if DB stringStore does not support strings longer than 64 bytes. If for say the store is only responsible for column names for example🤷.

I don't think there is a bug. The hash function only decides which bucket an element should be inserted into, not whether an element already exists in the set. As per cppreference.com:

Internally, the elements are not sorted in any particular order, but organized into buckets. Which bucket an element is placed into depends entirely on the hash of its value. This allows fast access to individual elements, since once a hash is computed, it refers to the exact bucket the element is placed into.

CCP-Aporia
CCP-Aporia previously approved these changes Jul 30, 2026

@ccp-serpent ccp-serpent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the registry needs to be configured back to carbonengine/vcpkg-registry. Additionally, I suggest a squash & merge strategy for this PR given that the changes are not particularly atomic and it's not healthy for the historic continuity of the component to have changes such as what's been applied to the vcpkg configuration to persist in history.

@ccptoebeans

Copy link
Copy Markdown
Member Author

Good idea with the squash and merge, and yes as soon as we're ready to merge the toolchain updates, I shall be changing the vcpkg-configuration back to carbonengine/vcpkg-registry 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants