Оцените лисапед
От: GhostCoders Россия  
Дата: 28.06.11 13:51
Оценка:
Здраствуйте!

Оцените плиз следующий лисапед.
Как положительные так и отрицательные качества.

Планирется Class1Impl сделать закрытым для пользователя (в другом заголовочном файле будет, тут только опережающее объявление оставить),
оставив для пользования только Class1.


class Class1Impl
{
    int mA;
    int mRefCount;

public:
    Class1Impl(): mA(0), mRefCount(1)
    {
        printf("Class1Impl::Class1Impl()\n");
    }

    virtual ~Class1Impl()
    {
        printf("~Class1Impl::Class1Impl()\n");
    }

    virtual void AddRef()
    { 
        mRefCount++; 
    }

    virtual void Release()
    { 
        mRefCount--; 
        if (mRefCount <= 0) 
            delete this; 
    }

    virtual void SetParamAImpl(int a)
    {
        mA = a;
    }

    virtual int GetParamAImpl() const
    {
        return mA;
    }
};

class Class1
{
    Class1Impl* mInternalImpl;

public:
    Class1()
    {
        mInternalImpl = new Class1Impl;
    }

    Class1(const Class1& other): mInternalImpl(other.mInternalImpl)
    {
        if (mInternalImpl)
            mInternalImpl->AddRef();
    }

    virtual ~Class1()
    {
        if (mInternalImpl)
            mInternalImpl->Release();
    }

    virtual bool operator == (const Class1 & other) const
    {
        return mInternalImpl == other.mInternalImpl;
    }

    virtual bool operator != (const Class1 & other) const
    {
        return mInternalImpl != other.mInternalImpl;
    }

    virtual Class1 & operator = (const Class1 & other)
    {
        if (mInternalImpl)
            mInternalImpl->Release();

        mInternalImpl = other.mInternalImpl;

        if (mInternalImpl)
            mInternalImpl->AddRef();

        return *this;
    }

    virtual void SetParamA(int a)
    {
        if (mInternalImpl)
            mInternalImpl->SetParamAImpl(a);
    }

    virtual int GetParamA() const
    {
        int result = 0;

        if (mInternalImpl)
            result = mInternalImpl->GetParamAImpl();

        return result;
    }
};

int _tmain(int argc, _TCHAR* argv[])
{
    Class1 a;
    Class1 b;
    Class1 c;
    a = c;

    printf("abc!\n");

    return 0;
}
Третий Рим должен пасть!
Re: Комментарии писать не пробовали?..
От: Erop Россия  
Дата: 28.06.11 17:39
Оценка: +1
Здравствуйте, GhostCoders, Вы писали:

GC>Оцените плиз следующий лисапед.

Нихрена не понятно зачем это вообще надо...
Все эмоциональные формулировки не соотвествуют действительному положению вещей и приведены мной исключительно "ради красного словца". За корректными формулировками и неискажённым изложением идей, следует обращаться к их автором или воспользоваться поиском
Re: Оцените лисапед
От: мыщъх США http://nezumi-lab.org
Дата: 28.06.11 17:43
Оценка: +1 -1
Здравствуйте, GhostCoders, Вы писали:

ужас.

int result = 0;

if (mInternalImpl)
result = mInternalImpl->GetParamAImpl();

return result;

так бы и писали int result = NOT_IMPL, а то ведь рехнуться можно.
americans fought a war for a freedom. another one to end slavery. so, what do some of them choose to do with their freedom? become slaves.
Re: Оцените лисапед
От: okman Беларусь https://searchinform.ru/
Дата: 28.06.11 17:55
Оценка: 2 (1) +2 :)
Здравствуйте, GhostCoders, Вы писали:

GC>Здраствуйте!


GC>Оцените плиз следующий лисапед.

GC>Как положительные так и отрицательные качества.

GC>Планирется Class1Impl сделать закрытым для пользователя (в другом заголовочном файле будет, тут только опережающее объявление оставить),

GC>оставив для пользования только Class1.


GC>
GC>class Class1Impl
GC>{
GC>    int mA;
GC>    int mRefCount;

GC>public:
GC>    Class1Impl(): mA(0), mRefCount(1)
GC>    {
GC>        printf("Class1Impl::Class1Impl()\n");
GC>    }

GC>    virtual ~Class1Impl()
GC>    {
GC>        printf("~Class1Impl::Class1Impl()\n");
GC>    }

GC>    virtual void AddRef()
GC>    { 
GC>        mRefCount++; 
GC>    }

GC>    virtual void Release()
GC>    { 
GC>        mRefCount--; 
GC>        if (mRefCount <= 0) 
GC>            delete this; 
GC>    }

GC>    virtual void SetParamAImpl(int a)
GC>    {
GC>        mA = a;
GC>    }

GC>    virtual int GetParamAImpl() const
GC>    {
GC>        return mA;
GC>    }
GC>};

GC>class Class1
GC>{
GC>    Class1Impl* mInternalImpl;

GC>public:
GC>    Class1()
GC>    {
GC>        mInternalImpl = new Class1Impl;
GC>    }

GC>    Class1(const Class1& other): mInternalImpl(other.mInternalImpl)
GC>    {
GC>        if (mInternalImpl)
GC>            mInternalImpl->AddRef();
GC>    }

GC>    virtual ~Class1()
GC>    {
GC>        if (mInternalImpl)
GC>            mInternalImpl->Release();
GC>    }

GC>    virtual bool operator == (const Class1 & other) const
GC>    {
GC>        return mInternalImpl == other.mInternalImpl;
GC>    }

GC>    virtual bool operator != (const Class1 & other) const
GC>    {
GC>        return mInternalImpl != other.mInternalImpl;
GC>    }

GC>    virtual Class1 & operator = (const Class1 & other)
GC>    {
GC>        if (mInternalImpl)
GC>            mInternalImpl->Release();

GC>        mInternalImpl = other.mInternalImpl;

GC>        if (mInternalImpl)
GC>            mInternalImpl->AddRef();

GC>        return *this;
GC>    }

GC>    virtual void SetParamA(int a)
GC>    {
GC>        if (mInternalImpl)
GC>            mInternalImpl->SetParamAImpl(a);
GC>    }

GC>    virtual int GetParamA() const
GC>    {
GC>        int result = 0;

GC>        if (mInternalImpl)
GC>            result = mInternalImpl->GetParamAImpl();

GC>        return result;
GC>    }
GC>};

GC>int _tmain(int argc, _TCHAR* argv[])
GC>{
GC>    Class1 a;
GC>    Class1 b;
GC>    Class1 c;
GC>    a = c;

GC>    printf("abc!\n");

GC>    return 0;
GC>}
GC>


Навскидку:

1. Оператор присваивания Class1 реализован некорректно, потому что не
отрабатывает такую ситуацию:

Class1 a;
a = a;


2. В GetParamA, наверное, лучше во избежание двусмысленности, кидать исключение,
если mInternalImpl равен нулю.

3. Конструктор Class1 можно маленько соптимизировать, переместив создание
класса имплементации в список инициализации:

public:
    Class1() : mInternalImpl(new Class1Impl) {}


А так вроде все в порядке.
Re[2]: Оцените лисапед
От: dZentle_man  
Дата: 28.06.11 20:20
Оценка:
Здравствуйте, okman, Вы писали:


O>1. Оператор присваивания Class1 реализован некорректно, потому что не

O>отрабатывает такую ситуацию:
Class1 a;
a = a;

O>А так вроде все в порядке.
Если уж дотрахиваться до таких мелочей, то надо и на переполнение проверять)
    int mRefCount;
    virtual void AddRef()
    { 
        mRefCount++; 
    }
Re: Оцените лисапед
От: dZentle_man  
Дата: 28.06.11 20:23
Оценка: +2 -1 :))
Здравствуйте, GhostCoders, Вы писали:

GC>Здраствуйте!


GC>Оцените плиз следующий лисапед.

"Что это было, Пух?" (с)
Завидую людям, у которых столько свободного времени, что они могут заниматься всякими глупостями, у которых нет ни практической цели, ни вообще какой-либо цели, кроме как "поиспражняться в кодинге".
Re[3]: Оцените лисапед
От: okman Беларусь https://searchinform.ru/
Дата: 28.06.11 20:25
Оценка:
Здравствуйте, dZentle_man, Вы писали:

Z_>Если уж дотрахиваться до таких мелочей, то надо и на переполнение проверять)


Дьявол в мелочах.
Re[2]: Оцените лисапед
От: bkat  
Дата: 28.06.11 21:17
Оценка: -1
Здравствуйте, dZentle_man, Вы писали:

Z_>Здравствуйте, GhostCoders, Вы писали:


GC>>Здраствуйте!


GC>>Оцените плиз следующий лисапед.

Z_>"Что это было, Пух?" (с)
Z_>Завидую людям, у которых столько свободного времени, что они могут заниматься всякими глупостями, у которых нет ни практической цели, ни вообще какой-либо цели, кроме как "поиспражняться в кодинге".

Очевидно же, что студент или что-то типа этого.
Самое время для написания лисапедов.
Re: Оцените лисапед
От: rg45 СССР  
Дата: 28.06.11 21:26
Оценка: -1
Здравствуйте, GhostCoders, Вы писали:

GC>Планирется Class1Impl сделать закрытым для пользователя (в другом заголовочном файле будет, тут только опережающее объявление оставить),

GC>оставив для пользования только Class1.
GC>...

Ровно того же можно добиться и мирным путем: http://ideone.com/LaD3G
--
Re: Оцените лисапед
От: novitk США  
Дата: 28.06.11 21:54
Оценка: 2 (1)
Здравствуйте, GhostCoders, Вы писали:

GC>Оцените плиз следующий лисапед.


Отсюда и до обеда.
Re[3]: Оцените лисапед
От: k.o. Россия  
Дата: 29.06.11 11:58
Оценка:
Здравствуйте, dZentle_man, Вы писали:

Z_>Здравствуйте, okman, Вы писали:



O>>1. Оператор присваивания Class1 реализован некорректно, потому что не

O>>отрабатывает такую ситуацию:
Z_>
Z_>Class1 a;
Z_>a = a;
Z_>

O>>А так вроде все в порядке.
Z_>Если уж дотрахиваться до таких мелочей, то надо и на переполнение проверять)

Ничего себе мелочь, UB в результате обычного вызова оператора присваивания. Это же совершенно обычный use-case. А для переполнения счётчика, нужно создать 2^16 объектов являющихся копией друг-друга, что, во-первых, выглядит, мягко говоря, странно, а во-вторых, невозможно (без дополнительных усилий) на машинах с разрядностью менее 64 бит.
Re[4]: Оцените лисапед
От: dZentle_man  
Дата: 29.06.11 15:52
Оценка: +2
Здравствуйте, k.o., Вы писали:


O>>>1. Оператор присваивания Class1 реализован некорректно, потому что не

O>>>отрабатывает такую ситуацию:
Z_>>
Z_>>Class1 a;
Z_>>a = a;
Z_>>

O>>>А так вроде все в порядке.
Z_>>Если уж дотрахиваться до таких мелочей, то надо и на переполнение проверять)

KO>Ничего себе мелочь, UB в результате обычного вызова оператора присваивания. Это же совершенно обычный use-case. А для переполнения счётчика, нужно создать 2^16 объектов являющихся копией друг-друга, что, во-первых, выглядит, мягко говоря, странно, а во-вторых, невозможно (без дополнительных усилий) на машинах с разрядностью менее 64 бит.

Это действительно мелочи, в сравнении с тем, что, как говорят китайцы, "а нахуа" вообще этот код?
Re[2]: Оцените лисапед
От: GhostCoders Россия  
Дата: 29.06.11 17:28
Оценка: :)
Здравствуйте, novitk, Вы писали:

N>Отсюда и до обеда.


а как сейчас поживает эта идиома? Насколько популярна? Есть ли в ней смысл?

Недостаток я вижу в том, что необходимо вручную дописывать свой враппер для каждого метода.

Хотя можно переопределить -> оператор.

Для чего это нужно? Хотелось бы иметь умный указатель с подсчетом ссылок, который был бы свободен от следующего бага:

struct A
{
//....
};

//...

void someFunc()
{
    boost::shared_ptr<A> sa(new A());
    boost::shared_ptr<A> sb(sa.get()); // Будет двойное удаление A
//...
}
Третий Рим должен пасть!
Re[3]: Оцените лисапед
От: Son of Northern Darkness  
Дата: 29.06.11 17:46
Оценка: 2 (1) +3
Здравствуйте, GhostCoders, Вы писали:

GC>
GC>struct A
GC>{
GC>//....
GC>};

GC>//...

GC>void someFunc()
GC>{
GC>    boost::shared_ptr<A> sa(new A());
GC>    boost::shared_ptr<A> sb(sa.get()); // Будет двойное удаление A
GC>//...
GC>}

GC>


Просто не надо так делать, вот и все. Если очень хочется, то для этого есть intrusive_ptr.
Re[3]: Оцените лисапед
От: novitk США  
Дата: 29.06.11 17:53
Оценка: +1 -1
Здравствуйте, GhostCoders, Вы писали:

GC>а как сейчас поживает эта идиома? Насколько популярна?

Достаточно.

GC>Есть ли в ней смысл?

Если ты задаешь этот вопрос, тебе оно не нужно.

GC>Недостаток я вижу в том, что необходимо вручную дописывать свой враппер для каждого метода.

Недостатков вагон и маленькая тележка.

GC>Хотя можно переопределить -> оператор.

Это дзен которого боятся маги 80-го уровня, а ты пока даже не на 5-м.
Даже не думай!

GC>Для чего это нужно? Хотелось бы иметь умный указатель с подсчетом ссылок, который был бы свободен от следующего бага:


GC> boost::shared_ptr<A> sa(new A());

GC> boost::shared_ptr<A> sb(sa.get()); // Будет двойное удаление A

Это не баг, а непонимание.
boost::shared_ptr<A> sb(sa);

Pimpl к умным ссылкам отношение прямого не имеет, он нужен для скрытия реализации.
Re[3]: Оцените лисапед
От: Erop Россия  
Дата: 29.06.11 18:59
Оценка: 2 (1) +1
Здравствуйте, GhostCoders, Вы писали:

GC>Для чего это нужно? Хотелось бы иметь умный указатель с подсчетом ссылок, который был бы свободен от следующего бага:



просто делаешь интрузивный подсчёт ссылок и будет тебе счастье...
Все эмоциональные формулировки не соотвествуют действительному положению вещей и приведены мной исключительно "ради красного словца". За корректными формулировками и неискажённым изложением идей, следует обращаться к их автором или воспользоваться поиском
Re[3]: Оцените лисапед
От: Erop Россия  
Дата: 29.06.11 19:15
Оценка: 2 (1)
Здравствуйте, GhostCoders, Вы писали:

GC>Недостаток я вижу в том, что необходимо вручную дописывать свой враппер для каждого метода.



Вообще, это конечно некоторая проблема PImpl в С++.
Но решение ТС ничего в ней не решает.

Есть два известных мне пути.

Путь 1. Описываем общую часть как-то так, что это описание используется ДВАЖДЫ.
Один раз для того, чтобы нагенерить публичную часть интерфейса, а другой раз для того, чтобы нагенерить часть реализации.

ну типа пишется что-то типа:
// хедер MyClass.h
DECLARE_PIMPL( MyClass )
    METHOD( type1, Method1 )
    METHOD1( type2, Method2, arg1_t )
    //....
END_OF_PIMPL()
и
//  хедер MyClassImpl.h
class MyClass::MyClassimpl {
public:
#include<BeginImpl.h>
#include "MyClass.h"
#include<EndImpl.h>
private:
// тут, собственно private часть реализщации
};


Путь 2.
Пишется свой препроцессор, который по как-то размеченному хедеру с описанием реализации генерит хедер с реализацией PImpl- обёртки.
Все эмоциональные формулировки не соотвествуют действительному положению вещей и приведены мной исключительно "ради красного словца". За корректными формулировками и неискажённым изложением идей, следует обращаться к их автором или воспользоваться поиском
Re: Оцените лисапед
От: IROV..  
Дата: 29.06.11 19:44
Оценка: 2 (1) +2
Здравствуйте, GhostCoders, Вы писали:

Factory + Interface + IntrusivePtr?
я не волшебник, я только учусь!
Re[5]: Оцените лисапед
От: k.o. Россия  
Дата: 29.06.11 20:04
Оценка: +1
Здравствуйте, dZentle_man, Вы писали:

Z_>Здравствуйте, k.o., Вы писали:


Z_>Это действительно мелочи, в сравнении с тем, что, как говорят китайцы, "а нахуа" вообще этот код?


Да ладно, обыкновенный pimpl, что тут непонятного-то? Есть, конечно, проблемы такие как изобретение велосипеда и нарушение SRP, но с кодом вполне можно работать. При необходимости он легко рефакторится до удобоваримого состояния. А вот отлавливать баги вызванные такой реализацией оператора присваивания бывает совсем не весело. В зависимости от везения, проявления могут быть разной степени фееричности. Во всяком случае, эта проблема на порядки отличается от гипотетически возможного переполнения счётчика ссылок.
Re[6]: Оцените лисапед
От: dZentle_man  
Дата: 29.06.11 21:18
Оценка: +1 -1
Здравствуйте, k.o., Вы писали:


Z_>>Это действительно мелочи, в сравнении с тем, что, как говорят китайцы, "а нахуа" вообще этот код?


KO>Да ладно, обыкновенный pimpl, что тут непонятного-то? Есть, конечно, проблемы такие как изобретение велосипеда и нарушение SRP, но с кодом вполне можно работать. При необходимости он легко рефакторится до удобоваримого состояния. А вот отлавливать баги вызванные такой реализацией оператора присваивания бывает совсем не весело. В зависимости от везения, проявления могут быть разной степени фееричности. Во всяком случае, эта проблема на порядки отличается от гипотетически возможного переполнения счётчика ссылок.

Лично у меня на второй строчке этого кода забывается что написано на первой. Еще отцы-основателиКерниган с Ритчи на первых же страницах говорили, что переменным и функциям нужно давать как можно более осмысленные имена. Не, ну не как у гугла в его глюгловом движке V8 — название функции в 42 буквы, а 2-3 слова, передающие основную суть, некоторые слова можно даже сократить. Че блин за переменная mA? Мой мозг отказывается такое воспринимать принципиально, это уже признак говнокода — дальше смотреть можно, но нужно ли? Я еще понимаю по работе, когда за это деньги платят, и то повбывав бы, но за просто так себя насиловать? С трудом вынудил себя найти ту же архитектурно заложенную опасность целочисленного переполнения, но целиком анализировать всю идею кода, а тем более просчитывать все возможные ошибки... "На фиг, на фиг — к терапевту" (с) При таком похабном написании в коде еще и ни одного коммента, которые призваны как-то сглаживать проблемы понимания когда ничего больше не помогает. Препод-стайл — "я написал, я такой умный, а вы втыкайте в мои скрижали". Надо иметь какое-то уважение к читателям если хочешь нормальной реакции.
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.