мой факап с умными указателями
От: Кодт Россия  
Дата: 02.12.23 01:15
Оценка: 7 (2) +1
Представим себе вот такой дистиллированный говнокод
struct IFace { ..... };

class TheImpl : public IFace { ..... };

class TheFixture {
public:
    IFace& iface() { return aggregate_; }
private:
    TheImpl aggregate_;
    // и ещё всякий клей, вспомогательные объекты, связи и функции
};

class TheProvider {
public:
    std::shared_ptr<IFace> provide() {
        if (need_create())
            create_cached_object();
        return cached_object_;
    }

private:
    std::shared_ptr<IFace> cached_object_;

    bool need_create() {
        return !cached_object_ || ....;  // кеша нет, кеш протух, всё такое...
    }

    // вот тут живёт бажок!
    void create_cached_object() {
        auto cached_object = std::make_shared<TheFixture>();
        cached_object_ = std::shared_ptr<IFace>{cached_object_, &cached_object->iface()};
    }
};


shared_ptr позволяет владеть объектом-сборкой, отдавая интерфейс на агрегат.
Например, указатель на узел дерева или даже на содержимое узла дерева.

https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr — сигнатура номер 8, "aliasing constructor"

Это даёт возможности очень гибко создавать всякие нетривиальные реализации интерфейсов.

Проблема в том, что нет никакого контроля — ни типа, ни значения аргументов такого конструктора.
Можно делать вот такие странные указатели: https://gcc.godbolt.org/z/ncz88ovW4

Тем не менее, заниматься самоприсваиванием с изменением указателя на агрегат — вполне легально и даже может иметь практический смысл:
https://gcc.godbolt.org/z/edEzMe953

Но с такой же лёгкостью мы можем получить преждевременный конец времени жизни!
Что я и получил, сделав опечатку в реальном проекте!

Здесь привожу иллюстрацию без NDA-подробностей.

Дистиллированный код — с багом и с исправлением:
https://gcc.godbolt.org/z/x53daYc5P
Перекуём баги на фичи!
Re: мой факап с умными указателями
От: so5team https://stiffstream.com
Дата: 02.12.23 08:52
Оценка: 1 (1) +7
Здравствуйте, Кодт

Т.е., по сути, ошибка была вызвана опечаткой в одном символе?

К>
К>        cached_object_ = std::shared_ptr<IFace>{cached_object_, &cached_object->iface()};
К>


Вместо:
cached_object_ = std::shared_ptr<IFace>{cached_object, &cached_object->iface()};


?

Это напомнило мне как несколько лет назад попробовал перейти с именования членов класса с префиксом "m_" на суффикс "_". Т.е. попробовал вместо "m_cached_object" использовать "cached_object_". Ну типа модно, молодежно и нет отсылок к венгерской нотации.

Но быстро убедился, что этот самый суффикс "_" очень плохо читается в выражениях, поэтому так и продолжил жить с "m_".

Что-то мне кажется, что если бы изначально код писался так:
m_cached_object = std::shared_ptr<IFace>{m_cached_object, &cached_object->iface()};

то опечатка, приведшая к багу, была бы более заметна.
Re: мой факап с умными указателями
От: reversecode google
Дата: 02.12.23 09:46
Оценка: +1
что сказали известные статик анализеры?
Re[2]: мой факап с умными указателями
От: Кодт Россия  
Дата: 04.12.23 11:28
Оценка:
Здравствуйте, so5team, Вы писали:

S>Т.е., по сути, ошибка была вызвана опечаткой в одном символе?


По сути — в неудачном названии. Если бы я назвал свежесозданную сборку не cached_object, а fixture...
auto fixture = std::make_shared<TheFixture>(1,2,3);
auto cached_object = std::shared_ptr<IFace>{fixture, &fixture->iface()};
cached_object_ = cached_object;


А по более глубокой сути — в объединении чистой функции (конструирование сборки и получение указателя) и модификатора объекта (переприсваивание кеша).

Разнёс по красоте — грабельность резко уменьшилась.
Перекуём баги на фичи!
Re[2]: мой факап с умными указателями
От: Кодт Россия  
Дата: 04.12.23 11:58
Оценка:
Здравствуйте, reversecode, Вы писали:

R>что сказали известные статик анализеры?


cppcheck и clang scan-build ничего не нашли.
Перекуём баги на фичи!
Re[3]: мой факап с умными указателями
От: reversecode google
Дата: 04.12.23 12:08
Оценка:
cppckeck слаб
clang не про это
нужен или PVS или coverty

pvs любит видеть такие странные подставки в именах переменных
Re: мой факап с умными указателями
От: sergii.p  
Дата: 04.12.23 19:17
Оценка:
Здравствуйте, Кодт, Вы писали:

немного оффтопик, но вроде для кеша лучше подходит weak_ptr

struct TheProvider {
    explicit TheProvider() {}

    std::shared_ptr<IFace> provide() {
        if(auto obj = cached_object_.lock()) {
            return obj;
        }
        else {
            auto fixture = std::make_shared<TheFixture>(1, 2, 3);
            cached_object_= std::shared_ptr<IFace>{fixture, &fixture->iface()};
            return cached_object_.lock();
        }
    }

private:
    std::weak_ptr<IFace> cached_object_;
};


И отдельный метод для нового конструктора тоже напрашивается. Только страшно его представить. Что-то типа такого:

template<typename F, typename I>
std::shared_ptr<I> make_shared_8(std::shared_ptr<F>& f, I& (F::*fn_ptr)()) {
    auto& i = ((*f.get()).*fn_ptr)();
    return std::shared_ptr<I> {f, &i};
}

...

cached_object_= make_shared_8(fixture, &TheFixture::iface);
Re[2]: мой факап с умными указателями
От: Кодт Россия  
Дата: 04.12.23 21:19
Оценка:
Здравствуйте, sergii.p, Вы писали:

SP>немного оффтопик, но вроде для кеша лучше подходит weak_ptr


Я тему времени жизни и условий обновления кеша тут не стал трогать, потому что совсем оффтопик.
Для логики "если в сокете есть новое, создадим новое, а иначе нужно взять старое" weak не подходит.

SP>И отдельный метод для нового конструктора тоже напрашивается. Только страшно его представить. Что-то типа такого:


Это не конструктор, это каст.
Когда агрегатом является база, то мы пользуемся тем, что есть конструктор shared_ptr<Base>(shared_ptr<Derived>)
а для всяких затейливых случаев — shared_pointer_cast.

В данном случае мы не создаём новый объект (make_shared), а создаём новый указатель к существующему объекту.

Да, пожалуй, завести функцию
template<class T, class M>
auto shared_member_ptr_cast(shared_ptr<T> p, M&(T::*m)) { return shared_ptr<M>{p, &p->*m}; }


template<class T, class M>
auto shared_member_fun_cast(shared_ptr<T> p, M&(T::*m)()) { return shared_ptr<M>{p, &(p->*m)()}; }

template<class T, class M>
auto shared_member_fun_cast(shared_ptr<T> p, M*(T::*m)()) { return shared_ptr<M>{p, (p->*m)()}; }
Перекуём баги на фичи!
Re[2]: мой факап с умными указателями
От: Skorodum Россия  
Дата: 05.12.23 11:53
Оценка:
Здравствуйте, so5team, Вы писали:

S>Но быстро убедился, что этот самый суффикс "_" очень плохо читается в выражениях, поэтому так и продолжил жить с "m_".

+1, "_" как суффикс или префикс особенно плох для С++, где активно используются точки и "->".
Re[3]: мой факап с умными указателями
От: reversecode google
Дата: 05.12.23 12:03
Оценка:
на вкус и цвет...
я пробовал префикс подчеркивание впереди, как то глаза болят
поэтому суффиксом подчеркивание более красивее вышло
всяко лучше чем некоторые жабисты пихают везде this->
префикс m_ осталось только в одном проекте, который больше мне напоминает что это виндовый проект
Re[4]: мой факап с умными указателями
От: Skorodum Россия  
Дата: 05.12.23 13:16
Оценка:
Здравствуйте, reversecode, Вы писали:

R>префикс m_ осталось только в одном проекте, который больше мне напоминает что это виндовый проект

Префикс "m_" широко распространен, потому что это хорошо задокументированный стандард Qt.
Есть boost и Qt — либо_то, ЛибоДругое, все остальное от лукавого
 
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.