2
\$\begingroup\$

Note - I posted updated code; see version 2 of this question.

I'm writing something like the Unity's shader graph and I'm kinda stuck on a property panel.

I add a float property, which name is Float, and then I add a second float property, which also is named Float (until I rename it). I'd like the second property name to be rearranged to Float(1)

What I tried at the beginning was a simple vector of properties

struct Property
{
    Property(std::string_view name, std::any value)
        : name(name), value(std::move(value)) {}

    std::string name;
    std::any value;
};

template<typename PropertyT>
void Add(std::string_view name, PropertyT&& value)
{
    static uint32_t counter = 0;
    auto finalName = counter == 0 ? name : fmt::format("{}({})", name, counter);
    properties.emplace_back(finalName, std::move(value));
    ++counter;
}

void Remove(std::string_view name)
{
    std::erase_if(properties, [name](const auto& prop){ return prop.name == name; });
}

std::vector<Property> properties;

but the static uint32_t counter is not a valid solution, because

  1. static variable
  2. the counter is never decremented, so if Float(1) will be named to something else, the next float property should be Float(1), not Float(2).

so I thought I'd do a map of the property pool by name.

struct Property
{
    Property(std::string_view name, std::any&& value, size_t index)
        : name(name), value(std::move(value))
    {
        if (index != 0) {
            this->name += "(" + std::to_string(index) + ")";
        }
    }

    const bool isValid() const
    {
        return value.has_value() && !name.empty();
    }

    std::any value;
    std::string name;
private:
    friend struct PropertyPool;

    int64_t next = 0;
};

struct PropertyPool
{
    [[nodiscard]] inline size_t Add(std::string_view name, std::any&& value)
    {
        if(nextFree == -1) {
            const size_t index = (size_t)nextFree;
            nextFree = pool[index].next;
            auto& prop = pool[index];
            prop = Property(name, std::move(value), index);
            nameToId[prop.name] = index;
            return index;
        } else {
            const size_t index = pool.size();
            auto& prop = pool.emplace_back(name, std::move(value), index);
            nameToId[prop.name] = index;
            return index;
        } 
    }

    void erase(size_t index)
    {
        auto& prop = pool[index];
        prop.name.clear();
        prop.value.reset();
        prop.next = nextFree;
        nextFree = (int64_t)index;
    }

    Property* tryGetByName(const std::string& name)
    {
        if(!nameToId.contains(name)) {
            return nullptr;
        }

        const size_t index = nameToId.at(name);
            
        if(pool.size() <= index) {
            return nullptr;
        }

        if(!pool[index].isValid()) {
            return nullptr;
        }

        return &pool[index];
    }

    std::unordered_map<std::string, int64_t> nameToId;
    std::vector<Property> pool;
    int64_t nextFree = -1;
};

struct PropertySet
{
    template<typename T>
    Property& Add(std::string_view name, T&& value)
    {
        const uint32_t hash = getHash(name);
        auto& pool = pools[hash];
        size_t index = pool.Add(name, std::move(value));
        return pool.pool[index];
    }

    void Remove(std::string_view name)
    {
        const uint32_t hash = getHash(name);
        auto& pool = pools[hash];
        pool.erase(pool.nameToId[std::string(name)]);
    }

    const uint32_t getHash(std::string_view name) const
    {
        auto it = name.find_last_of("(");
        if (it != std::string_view::npos) {
            name.remove_suffix(name.size() - it);
        }
        return std::hash<std::string_view>{}(name);
    }
    std::unordered_map<uint32_t, PropertyPool> pools;
};

simple main:

#include <unordered_map>
#include <vector>
#include <string_view>
#include <string>
#include <any>
#include <functional>

int main()
{
    PropertySet properties;
    
    properties.Add("Float", 2.0f);
    properties.Add("Float", 3.0f);
    properties.Add("Float", 0.32f);

    properties.Remove("Float(1)");

    properties.Add("Float", 0.84);
    
}

expected values:

Float
Float(1)
Float(2)

after Remove

Float
Float(2)

add after remove

Float
Float(1)
Float(2)

The code looks awful, but it more or less works as I wish.

The main limitation is that this whole code is written only to support properties with the same names, that will most probably be renamed. Thus this solution is kinda useless because let's say there are 4 properties named: Float, x, y, z. The std::unordered_map<uint32_t, PropertyPool> pools will allocate memory, and the PropertyPool will also allocate memory, so it's kinda waste.

What do you think about it? How would you implement something like this? What would you change in my code? (I bet everything :D, but keep it for real, though)

\$\endgroup\$
4
  • \$\begingroup\$ This can't really work as posted. When you Add the first value, nextFree has the value of -1, which is then copied to index which is then used here: nextFree = pool[index].next; which is guaranteed to segfault. \$\endgroup\$
    – Edward
    Nov 12, 2022 at 18:18
  • \$\begingroup\$ I've just edited the code. Haven't reviewed what was wrong with the old one (I bet I've messed something up while editing this post). I have tested it and it works now. \$\endgroup\$
    – Edziju
    Nov 12, 2022 at 18:22
  • \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Nov 12, 2022 at 20:13
  • \$\begingroup\$ @Edward, "guaranteed to segfault" is an exaggeration - it depends strongly on the target platform. \$\endgroup\$ Nov 12, 2022 at 20:15

1 Answer 1

4
\$\begingroup\$

The #include lines are in the wrong place - they really need to be included before the Property class which uses them.

The following identifiers live in namespace std:

  • std::size_t
  • std::int64_t
  • std::uint32_t

I'm guessing your compiler has exercised its privilege to also define global-namespace equivalents, but portable code can't depend on that, so properly qualify the names where you use them.

Also, consider whether you really need those exact-width types (and should therefore fail to compile on platforms that don't have them), or whether possibly-larger types would satisfy your needs - e.g. std::int_fast64_t.

Some compiler warnings that are easily fixed:

280954.cpp:19:5: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
   19 |     const bool isValid() const
      |     ^~~~~
280954.cpp: In constructor ‘Property::Property(std::string_view, std::any&&, std::size_t)’:
280954.cpp:25:17: warning: ‘Property::name’ will be initialized after [-Wreorder]
   25 |     std::string name;
      |                 ^~~~
280954.cpp:24:14: warning:   ‘std::any Property::value’ [-Wreorder]
   24 |     std::any value;
      |              ^~~~~
280954.cpp:11:5: warning:   when initialized here [-Wreorder]
   11 |     Property(std::string_view name, std::any&& value, std::size_t index)
      |     ^~~~~~~~
280954.cpp: At global scope:
280954.cpp:102:5: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
  102 |     const std::uint32_t getHash(std::string_view name) const
      |     ^~~~~
280954.cpp: In member function ‘const uint32_t PropertySet::getHash(std::string_view) const’:
280954.cpp:108:45: warning: conversion from ‘std::size_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
  108 |         return std::hash<std::string_view>{}(name);
      |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
280954.cpp: In constructor ‘PropertySet::PropertySet()’:
280954.cpp:84:8: warning: ‘PropertySet::pools’ should be initialized in the member initialization list [-Weffc++]
   84 | struct PropertySet
      |        ^~~~~~~~~~~
280954.cpp:32:8: warning: ‘PropertyPool::nameToId’ should be initialized in the member initialization list [-Weffc++]
   32 | struct PropertyPool
      |        ^~~~~~~~~~~~
280954.cpp:32:8: warning: ‘PropertyPool::pool’ should be initialized in the member initialization list [-Weffc++]

The test program fails on my system:

==795531== Invalid read of size 8
==795531==    at 0x10BDDC: PropertyPool::Add(std::basic_string_view<char, std::char_traits<char> >, std::any&&) (280954.cpp:38)
==795531==    by 0x10D2EC: Property& PropertySet::Add<float>(std::basic_string_view<char, std::char_traits<char> >, float&&) (280954.cpp:91)
==795531==    by 0x10B340: main (280954.cpp:118)
==795531==  Address 0xfffffffffffffff8 is not stack'd, malloc'd or (recently) free'd
==795531== 
==795531== 
==795531== Process terminating with default action of signal 11 (SIGSEGV)
==795531==  Access not within mapped region at address 0xFFFFFFFFFFFFFFF8
==795531==    at 0x10BDDC: PropertyPool::Add(std::basic_string_view<char, std::char_traits<char> >, std::any&&) (280954.cpp:38)
==795531==    by 0x10D2EC: Property& PropertySet::Add<float>(std::basic_string_view<char, std::char_traits<char> >, float&&) (280954.cpp:91)
==795531==    by 0x10B340: main (280954.cpp:118)

It's hard to review further until this is fixed.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.