dfhack icon indicating copy to clipboard operation
dfhack copied to clipboard

improve `PersistentDataItem`

Open ab9rf opened this issue 2 years ago • 3 comments

While working on autoclothing I noticed myself going to cut and paste code from another plugin in order to accomplish something, which means we probably need to centralize something. The specific code is

static int get_config_val(PersistentDataItem &c, int index) {
    if (!c.isValid())
        return -1;
    return c.ival(index);
}
static bool get_config_bool(PersistentDataItem &c, int index) {
    return get_config_val(c, index) == 1;
}
static void set_config_val(PersistentDataItem &c, int index, int value) {
    if (c.isValid())
        c.ival(index) = value;
}
static void set_config_bool(PersistentDataItem &c, int index, bool value) {
    set_config_val(c, index, value ? 1 : 0);
}

All of these could be member functions of PersistentDataItem. Furthermore, I am fairly certain that we could use operator overloads to implement all of these so that they would allow PersistentDataItems to be used with natural syntax, which would probably be a general improvement as well as reducing code duplication.

At the very least, we should probably add these to Persistence.h but I think it's probably best to incorporate them into the class definition in an intelligent way.

ab9rf avatar Mar 25 '23 16:03 ab9rf

Possible solution:

int operator[](size_t index) const {
    if (!isValid())
        return -1;
    return ival(index);
}

int& operator[](size_t index) {
    return ival(index);
}

Also, we have to think about overloading some of access operapors to make isValid() check by default during access.

shevernitskiy avatar Aug 22 '23 03:08 shevernitskiy

i'm reopening this because, while the recent changes are beneficial, they don't appear to fully address the rationale behind this issue

my intention was to have PersistentDataItems to have such overloads that code can treat these essentially as if they were "ordinary variables" via appropriate overloads of things like operator= to allow essentially transparent use of PDIs. we're not there yet, which is why i've reopened this

ab9rf avatar Jan 14 '24 00:01 ab9rf

note: there is no reason for this to be a release blocker; this is about facilitating development velocity, not code correctness

i've seen some projects with a "some future version" project for things that "should be done" but which have no specific urgency, and this would be a good place to park something like this

ab9rf avatar Jan 14 '24 00:01 ab9rf