Код давали одной знакомой в качестве теста — судя по тому, что я полностью был с ней согласен — я ошибки не нашел тоже (или нашел, но не те

#include <windows.h>
#include <iostream>
#include <sstream>
#include <process.h>

namespace TST
{
    class Exception
    {
    public:
        Exception(const wchar_t* szwText)
            :   m_wstrText(szwText)
        {;};

        std::wstring What() const throw()
        {
            return m_wstrText;
        };

    protected:
        const std::wstring m_wstrText;
    };

    void le_chk(BOOL bResult, const char* file, int line)
    {
        if(!bResult)
        {
            std::clog << "An error " << GetLastError() << "occured";
            std::wostringstream os;
            os  << L"Win32 error occured: GetLastError returned " << GetLastError() 
                << " in " << file << L"@" << line;
            throw Exception(os.str().c_str());
        };
    };

    #define TST_LE_CHECK(res)   le_chk((res), __FILE__, __LINE__)

    class CallBase
    {
    public:
        virtual void Call() = 0;
    };

    class AsyncActionBase
    {
    public:
        AsyncActionBase(CallBase* pCall)
            :   m_hThread(NULL)
        {
            unsigned uId = 0;
            m_hThread = (HANDLE)_beginthreadex(
                            NULL, 0, &AsyncActionBase::thread_proc, 
                            (void*)pCall, 0, &uId);
            TST_LE_CHECK(!!m_hThread);
        };

        virtual ~AsyncActionBase()
        {
            if(m_hThread)
            {
                TST_LE_CHECK(WaitForSingleObject(m_hThread, INFINITE) 
                    != WAIT_FAILED);
                TST_LE_CHECK(CloseHandle(m_hThread));
                m_hThread = NULL;
            };
        };
    protected:
        static unsigned __stdcall thread_proc(void * p)
        {
            try
            {
                ((CallBase*)p)->Call();
            }
            catch (const TST::Exception& excpt)
            {
                std::wcerr << excpt.What();
            };
            return 0;
        };
        HANDLE m_hThread;
    };

    class MyAsyncAction1
        :   public CallBase
        ,   public AsyncActionBase
    {
    public:
        MyAsyncAction1()
            :   AsyncActionBase(this)
        {
            ;
        };
        
        virtual ~MyAsyncAction1()
        {
            ;
        };

    //CallBase
        virtual void Call()
        {
            for(size_t i = 0; i < 100; ++i)
            {
                std::cout << "Test #" << (i+1) << std::endl;
                Sleep(10);
            };
        };
    };
};

int main()
{
    try
    {
        TST::MyAsyncAction1 oBj;
    }
    catch (const TST::Exception& excpt)
    {
        std::wcerr << excpt.What();
    };
};

Здравствуйте, Alex Dav, Вы писали:

AD>Код давали одной знакомой в качестве теста — судя по тому, что я полностью был с ней согласен — я ошибки не нашел тоже (или нашел, но не те

я вижу проблему в конструкторе класса class MyAsyncAction1:
один и родителей в конструкторе запускает поток, который будет делать виртуальный вызов метода Call для объекта, который потенциально еще не сконструировался
в итоге будет pure virtual call

ну и метод std::wstring What() const throw() забавляет )
ведь он все же может кинуть исключение при копировании строки, хотя в объявлении прописано throw()

On 12/14/2011 06:10 PM, Alex Dav wrote:
> что вас смущает в данном коде? <message/4542120.aspx> Оценить

Как минимум, деструктор тут выбрасывает исключения.

> virtual ~AsyncActionBase()
> {
> if(m_hThread)
> {
> TST_LE_CHECK(WaitForSingleObject(m_hThread, INFINITE)
> != WAIT_FAILED);
> TST_LE_CHECK(CloseHandle(m_hThread));
> m_hThread = NULL;
> };
> };

Еще, похоже, GetLastError() поздновато вызывается.

On 12/14/2011 06:10 PM, Alex Dav wrote:

> что вас смущает в данном коде? <message/4542120.aspx> Оценить

Далее тут вот:

> class MyAsyncAction1
> :public CallBase
> ,public AsyncActionBase
> {
> public:
> MyAsyncAction1()
> : AsyncActionBase(this)
> {
> ;
> };

происходит следующее:

Вызывается конструктор предка AsyncActionBase() с передачей
туда this в качестве ссылки на CallBase.

Это не всегда можно делать, поскольку в этот момент объект,
на который указывает this, ещё не сконструирован
(будет только когда управление попадёт на '{', а лучше вообще
дождаться '}' )

Так можно делать, только если this сохраняется как ссылка для
дальнейшего использования в более познее время.

С учётом того, что AsyncActionBase тут же запустит поток с использованием
виртуальных фунций этого объекта (которые ещё не проинициализированы),
то очень вероятно, что будет либо нарушение защиты памяти, либо
"pure virtual func call", либо далее реализация CallBase будет
работать какое-то время со своими неинициализированными данными.

Вообще, сама идея делать что-то в конструкторе, кроме инициализации,
порочна, а уж запускать там поток -- вообще безумие.

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

lxa>Еще, похоже, GetLastError() поздновато вызывается.

Честно говоря не уверен, а где бы вы вызвали?

Здравствуйте, Alex Dav, Вы писали:

1) Конструктор Exception может кинуть исключение.
2) const std::wstring& Exception::What() const throw()
3) std::clog << "An error " << GetLastError() << "occured"
GetLastError может вернуть ошибку работы std::clog << "An error "
надо err = GetLastError(); std::clog << format(err); throw Exception(format(err));
А лучше переделать этот метод с минимальным использованием функций которые могут вызвать ошибку.
4) Конструктор AsyncActionBase(CallBase* pCall) вызывается с параметром this, указывающим на потомка, который еще не полностью создан, и таблица виртуальных методов не инициализирована, возможен вызов чисто виртуальной функции в запущенном потоке.
5) Деструктор ~AsyncActionBase() бросает исключение
6) в std::wcerr пишется с разных потоков без синхронизации и без << std::endl, бардак в консоли

Всё. Всего-то пара строчек кода, а непонятно зачем это всё, что оно делает. Имена неговорящие, комментариев нет, какие-то константы кругом.

Остальное всё вторично.

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

_>Здравствуйте, Alex Dav, Вы писали:

_>1) Конструктор Exception может кинуть исключение.
_>2) const std::wstring& Exception::What() const throw()
_>3) std::clog << "An error " << GetLastError() << "occured"
_> GetLastError может вернуть ошибку работы std::clog << "An error "
_> надо err = GetLastError(); std::clog << format(err); throw Exception(format(err));
_> А лучше переделать этот метод с минимальным использованием функций которые могут вызвать ошибку.
_>4) Конструктор AsyncActionBase(CallBase* pCall) вызывается с параметром this, указывающим на потомка, который еще не полностью создан, и таблица виртуальных методов не инициализирована, возможен вызов чисто виртуальной функции в запущенном потоке.
_>5) Деструктор ~AsyncActionBase() бросает исключение
_>6) в std::wcerr пишется с разных потоков без синхронизации и без << std::endl, бардак в консоли

4 и 5 вообще очень грубые ошибки. imho, если кандидат их не заметил, то его можно брать только на начинающего разработчика.
3 — тоже довольно известные грабли

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

B>4 и 5 вообще очень грубые ошибки. imho, если кандидат их не заметил, то его можно брать только на начинающего разработчика.
согласен
B>3 — тоже довольно известные грабли
ну как бы не критичная

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

RNS>Всё. Всего-то пара строчек кода, а непонятно зачем это всё, что оно делает. Имена неговорящие, комментариев нет, какие-то константы кругом.
Ага это первое, что было отмечено, но есть великие и ужасные конторы, которые считают это ... хм, красивым(?)

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

RNS>Имена неговорящие, комментариев нет, какие-то константы кругом.

Ну и что, ты бы на собеседовании так и сказал, код у вас какой-то голимый и некрасивый, хер разберешь, мне в лом?

Здравствуйте, Alex Dav, Вы писали:

AD>Код давали одной знакомой в качестве теста — судя по тому, что я полностью был с ней согласен — я ошибки не нашел тоже (или нашел, но не те :))

AD>#include <windows.h>
AD>#include <iostream>
AD>#include <sstream>
AD>#include <process.h>

AD>namespace TST
AD>{
AD>    class Exception
AD>    {
AD>    public:
AD>        Exception(const wchar_t* szwText)  // должен быть explicit
AD>            :   m_wstrText(szwText)
AD>        {;};  // лишние точки с запятой

AD>        std::wstring What() const throw()  // throw() зло
AD>        {
AD>            return m_wstrText;
AD>        };  // лишние точки с запятой

AD>    protected:
AD>        const std::wstring m_wstrText;
AD>    };

AD>    void le_chk(BOOL bResult, const char* file, int line)  // неудачное имя функции
AD>    {
AD>        if(!bResult)
AD>        {
AD>            std::clog << "An error " << GetLastError() << "occured";  // неконсистентная табуляция
AD>            std::wostringstream os;
AD>            os  << L"Win32 error occured: GetLastError returned " << GetLastError()  // два вызова GetLastError подряд, и не факт, что предыдущий вывод в clog на них не влияет
AD>                << " in " << file << L"@" << line;
AD>            throw Exception(os.str().c_str());
AD>        };  // лишние точки с запятой
AD>    };  // лишние точки с запятой

AD>    #define TST_LE_CHECK(res)   le_chk((res), __FILE__, __LINE__)

AD>    class CallBase
AD>    {
AD>    public:
AD>        virtual void Call() = 0;
AD>    };

AD>    class AsyncActionBase
AD>    {
AD>    public:
AD>        AsyncActionBase(CallBase* pCall)  // explicit
AD>            :   m_hThread(NULL)
AD>        {
AD>            unsigned uId = 0;
AD>            m_hThread = (HANDLE)_beginthreadex(  // C-style cast. Должен быть reinterpret_cast
AD>                            NULL, 0, &AsyncActionBase::thread_proc, 
AD>                            (void*)pCall, 0, &uId);  // C-style cast. Должен быть static_cast
AD>            TST_LE_CHECK(!!m_hThread);
AD>        };  // лишние точки с запятой

AD>        virtual ~AsyncActionBase()
AD>        {
AD>            if(m_hThread)
AD>            {
AD>                TST_LE_CHECK(WaitForSingleObject(m_hThread, INFINITE) 
AD>                    != WAIT_FAILED);  // деструкторы, бросающие исключения — зло
AD>                TST_LE_CHECK(CloseHandle(m_hThread));  // деструкторы, бросающие исключения — зло
AD>                m_hThread = NULL;
AD>            };  // лишние точки с запятой
AD>        };  // лишние точки с запятой
AD>    protected:
AD>        static unsigned __stdcall thread_proc(void * p)  // protected static — это для кого?
AD>        {
AD>            try
AD>            {
AD>                ((CallBase*)p)->Call();  // C-style cast. Должен быть static_cast
AD>            }
AD>            catch (const TST::Exception& excpt)
AD>            {
AD>                std::wcerr << excpt.What();
AD>            };  // лишние точки с запятой
AD>            return 0;
AD>        };  // лишние точки с запятой
AD>        HANDLE m_hThread;
AD>    };

AD>    class MyAsyncAction1
AD>        :   public CallBase
AD>        ,   public AsyncActionBase
AD>    {
AD>    public:
AD>        MyAsyncAction1()
AD>            :   AsyncActionBase(this)  // this ещё не сконструирован, а AsyncActionBase уже вызывает на него Call
AD>        {
AD>            ;  // лишние точки с запятой
AD>        };  // лишние точки с запятой
        
AD>        virtual ~MyAsyncAction1()
AD>        {
AD>            ;  // лишние точки с запятой
AD>        };  // лишние точки с запятой

AD>    //CallBase
AD>        virtual void Call()
AD>        {
AD>            for(size_t i = 0; i < 100; ++i)
AD>            {
AD>                std::cout << "Test #" << (i+1) << std::endl;  // лучше было бы крутить цикл от 1 до <=100 и выводить i
AD>                Sleep(10);
AD>            };  // лишние точки с запятой
AD>        };  // лишние точки с запятой
AD>    };
AD>};  // лишние точки с запятой

AD>int main()
AD>{
AD>    try
AD>    {
AD>        TST::MyAsyncAction1 oBj;
AD>    }
AD>    catch (const TST::Exception& excpt)
AD>    {
AD>        std::wcerr << excpt.What();
AD>    };  // лишние точки с запятой
AD>};  // лишние точки с запятой

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

B>4 и 5 вообще очень грубые ошибки. imho, если кандидат их не заметил, то его можно брать только на начинающего разработчика.
Если добавить синхронизацию, то можно избежать 4
Исключение в деструкторе это еще не катастрофа. Хотя в этом примере это катастрофа.

B>3 — тоже довольно известные грабли
А то что логирование не работает, по мне, более неприятнее 4 и 5. Лучше бы вообще не было такого "дезинформатора".

Я расписал то что выглядит некрасиво. А красота она для каждого своя

Какую то второстепенную фигню отметили, а главные проблемы не назвали.

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

RNS>Всё. Всего-то пара строчек кода, а непонятно зачем это всё, что оно делает. Имена неговорящие, комментариев нет, какие-то константы кругом.

А по моему, все понятно, что там делается. К стилю всегда можно попридираться.

RNS>Остальное всё вторично.

То есть для вас неговорящая константа и отсутствие комментариев более первично, чем вызов виртуального метода в деструкторе?

Здравствуйте, Alex Dav, Вы писали:

AD>Код давали одной знакомой в качестве теста — судя по тому, что я полностью был с ней согласен — я ошибки не нашел тоже (или нашел, но не те

Этот код мне напоминает старый анекдот:

Сидит программист глубоко в отладке.
Подходит сынишка:
— Папа, почему солнышко каждый день встает на востоке, а садится на западе?
— Ты это проверял?
— Проверял.
— Хорошо проверял?
— Хорошо.
— Работает?
— Работает.
— Каждый день работает?
— Да, каждый день.
— Тогда ради Бога, сынок, ничего не трогай, ничего не меняй!

Попытка в функции MyAsyncAction1::Call() использовать член класса MyAsyncAction1 приведёт к "неожиданным" проблемам...
Например, наивный программист глядя на этот класс:

    class MyAsyncAction1 : public CallBase, public AsyncActionBase
    {
    public:
        MyAsyncAction1() : AsyncActionBase(this)
        {
        };
       
        virtual ~MyAsyncAction1()
        {
        };

    //CallBase
        virtual void Call()
        {
            for(size_t i = 0; i < 100; ++i)
            {
                std::cout << "Test #" << (i+1) << std::endl;
                Sleep(10);
            };
        };
    };

может захотеть заменить в MyAsyncAction1::Call() поток std::cout на член класса (ну а что такого ? он же private!):

    class MyAsyncAction1 : public CallBase, public AsyncActionBase
    {
    public:
        MyAsyncAction1() : AsyncActionBase(this)
        {
        };
       
        virtual ~MyAsyncAction1()
        {
            std::cout << m_out.str();
        };

    //CallBase
        virtual void Call()
        {
            for(size_t i = 0; i < 100; ++i)
            {
                m_out << "Test #" << (i+1) << std::endl;
                Sleep(10);
            };
        };
    private:
        std::ostringstream m_out;
    };

то-то он будет удивлён, когда запустит этот вариант!

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

BFE>то-то он будет удивлён, когда запустит этот вариант!
я тоже удивлен, т.к. не понял суть (ошибку) в вашем коде =\

Здравствуйте, Alex Dav,

время жизни потока и объекта никак не связанны

а вообще это бред такие тесты имхо правильно делать так спросить в чем рубит
человек что может написать и потом дать задание написать программу чтобы
это заняло где то недельку и потом посмотреть что он там понапишет и как
имхо так все кто просто заучил кому не очень надо и прочие авантюристы
отсеятся и тут будет комплексная проверка а не только на вниманительность
сразу будет виден опыт человека а так просто влом искать сиплюсплюсные баги
специально засаженные в код главное когда ты сам пишеш не делать таких ляпов
имхо очень тупая проверка и тест в целом что можно после этого сказать о
соискателе ? ничего