1. C++ / Говнокод #1851

    +52.9

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    void SomeFunction(SomeClass* cls)
    {
        std::auto_ptr<SomeClass> tmp(cls);
        SomeObject.SomeMethod(tmp.release());
    }

    несколько раз видел такое в разных вариациях (поэтому вместо копи-пасты - абстракция).
    смысл сей конструкции упорно ускользает от меня :)

    Запостил: g26g, 16 Сентября 2009

    Комментарии (22) RSS

    • Если между 3-ей и 4-ой строками выполняются действия, которые могут привести к исключению (например, вывод в std::cout, сложение двух std::string-ов, и многие-многие-многие-многие другие), то объект, на который указывает cls будет удалён, т.е. не будет утечки памяти.

      Пример использования:

      SomeFunction(new SomeClass); // и всё, мы забыли, об этом указателе - нам не нужно беспокоиться об удалении объекта.

      Чтоб было совсем понятно, вот чуть расширенный пример (правильный):

      void SomeFunction(SomeClass* cls)
      {
      std::auto_ptr<SomeClass> tmp(cls);
      std::cout << "К нам приехал " << *tmp << std::endl; // может вылететь исключение, но объект будет удалён в деструкторе std::auto_ptr<SomeClass>::~std::auto_ptr <SomeClass>()
      SomeObject.SomeMethod(tmp.release()); // здесь мы "отпустли" объект, т.е. сами его удалять не будем - теперь это не наша проблема - его обязан удалить SomeObject.SomeMethod или передать тому, кто его удалит.
      }

      Теперь должно быть понятно, что НЕ ПРАВИЛЬНО писать так:

      void SomeFunction(SomeClass* cls)
      {
      std::cout << "К нам приехал " << *cls << std::endl; // вылетит исключение - объект станет ничей, т.е. будет утечка памяти
      SomeObject.SomeMethod(cls);
      }

      Читайте книжки, они рулез.
      Ответить
      • книжки на эту тему читать мне уже поздно, я давно с этим знаком, если что.
        хотя это развернутое объяснение безусловно может быть кому-то полезно (сорри, что заминусовал коммент - промахнулся :( )

        говнокод состоит в том, что
        1) между получением и отдачей владения ресурсом auto_ptr<> не делается ровно ни одной операции, которая даже теоретически может вызвать исключение (по факту - вообще ничего не делается)
        2) интерфейс функции небезопасен, т.к. ничто не мешает нам передать указатель на объект, который хранится по значению, например, со стека, не зная, что внутри им завладее кто-то другой
        Ответить
        • "смысл сей конструкции упорно ускользает" и "я давно с этим знаком" - что-то у тебя не вяжется.

          здесь и не пахнет говнокодом потому, что:

          1) то, что "не делается ровно ни одной операции" не гарантирует того, что такие операции не появятся в будущем. И они имеют на это право, в присутствии auto_ptr. Удали из этого примера auto_ptr и ты не имеешь права НИЧЕГО делать, кроме вызова единственного метода "SomeObject.SomeMethod". Или ты предлагаешь перед добавлением каждого std::cout << "Фигасе, нас вызвали!" лазить по всем исходникам/документации, смотреть как эту функцию вызывают и кто кем владеет? А тут ясно сказано, самодокументирующимся кодом, что функция владеет и шабаш. На то, что функция не принимает auto_ptr в качестве параметра - может быть причина - у тебя это не пропечатано;
          2) ага, интерфейс auto_ptr тоже "небезопасен" по этой же причине.
          Ответить
          • ок, поспорим еще

            1) "не гарантирует того, что такие операции не появятся в будущем"
            конечно не гарантирует, но захват владения ресурсом в этом случае - нелегален (хотя бы по причине пункта 2 в предыдущем посте). а что? давайте все заворачивать в авто_птр :) на всякий пожарный. мало ли что мы тут еще понапишем, а юзер потом будет думать, откуда у него memory corruption. бред.
            2) "не имеешь права НИЧЕГО делать"
            еще как имею, но ответственность за отлов исключений перекладываю на пользователя.
            или явно пишу void SomeFunction(...) throw() { ... }, избавляя от этого пользователя, и занимаюсь самостоятельной ловлей блох в теле функции.
            3) "А тут ясно сказано, самодокументирующимс я кодом, что функция владеет и шабаш"
            бред сивой кобылы. это имплементация - пользователь знает только то, что он может передать в нее голый указатель.
            4) "интерфейс auto_ptr тоже "небезопасен" по этой же причине"
            нет. он ясно дает понять пользователю, что тот добровольно отказывается от владения ресурсом.
            Ответить
            • 1)
              \"хотя бы по причине пункта 2 в предыдущем посте\"
              ого. начались передёргивания? 1-ый пункт на то и первый, чтобы отличаться от второго, а ты всё в кучу замесил.

              \"давайте все заворачивать в авто_птр :)\"
              не вижу ничего смешного. вообще-то, да - всё, что можно завернуть в auto_ptr, обязано быть туда завёрнуто.

              \"юзер потом будет думать, откуда у него memory corruption. бред\"

              См. пример с животными (эту ссылку уже приводил, но лишним, чувствую, не будет) - http://www.boost.org/doc/libs/1_40_0/libs/ptr_container/doc/tutorial.html. Возьми свой любимый почтовый клиент и прям сюда - [email protected] и напиши, дескать, \"бред\" у вас, уважаемые.

              2) \"void SomeFunction(...) throw()\"

              ого, круто. и часто у тебя такое? Саттера не читал пока? И что, внутри такой функции всё ловишь? И bad_alloc тоже? А потом что с ними делаешь? Прям кунсткамера получается. Для такого кода сайт специальный есть.

              3) все адреса я уже привёл выше, пиши туда, мелким почерком.

              4) \"он ясно дает понять\"
              Ну, покаж-ка пальцем, где это auto_ptr даёт понять?
              Вот его декларация:

              template<class X> class auto_ptr {
              public:
              typedef X element_type;
              explicit auto_ptr(X* p =0) throw();
              auto_ptr(auto_ptr&) throw();
              template<class Y> auto_ptr(auto_ptr<Y>&) throw();
              auto_ptr& operator=(auto_ptr&) throw();
              template<class Y> auto_ptr& operator=(auto_ptr<Y>&) throw();
              auto_ptr& operator=(auto_ptr_ref<X> r) throw();
              ˜auto_ptr() throw();
              X& operator*() const throw();
              X* operator->() const throw();
              X* get() const throw();
              X* release() throw();
              void reset(X* p =0) throw();
              auto_ptr(auto_ptr_ref<X>) throw();
              template<class Y> operator auto_ptr_ref<Y>() throw();
              template<class Y> operator auto_ptr<Y>() throw();
              };
              Ответить
              • я смотрю тут спор ради спора. товарищ, прекращайте уже, вы не правы. мне уже лень спорить, потом поймете все.

                "1-ый пункт на то и первый" - не надо выдергивать из контекста, было сказано "второй пункт _предыдущего_ поста"
                "всё, что можно завернуть в auto_ptr, обязано быть туда завёрнуто" - все, что может быть к месту завернуто - имеет быть право, иначе - нет. и ни разу не обязано.

                пример про boost::ptr_container неправильный, эти контейнеры создавались, чтобы контролировать жизнь указателей.
                возмущение по поводу auto_ptr неправильно - он создавался, чтобы контролировать жизнь указателя.
                всю эту информацию не утаивают от пользователей, а если они неправильно воспользовались такими инструментами - это исключительно их ошибка.
                Ответить
                • да, забыл проехаться по комментарию про throw() и bad_alloc.
                  bad_alloc никогда в жизни не ловил, есть исключения, которые должны ловиться на максимально верхнем логическом уровне и просто завершать работу приложения / задачи (с сохранением промежуточных данных, если надо). мало смысла в том, чтобы продолжать работать в условиях нехватки памяти - это чревато другими ошибками.

                  так вот, теперь по поводу throw() - такая спецификация ставится не просто так - это жесткое требование к коду внутри функции, если я не уверен в том, что отсюда не полетит исключение - не стану заявлять об этом в интерфейсе. тогда пользователь будет знать о том, что ему придется самому поддержать rollback/уничтожение динамически выделенных данных в случае исключения.

                  adios, amigo, в общем. читай книжки дальше, вдумчиво, а не по диагонали.
                  Ответить
                  • Про bad_alloc сложно. Его нужно трактовать как выдернутую из розетки вилку.

                    Пытаться выжить после него смысла нет, но иногда нужно умудриться как можно меньше нагадить ( (с сохранением промежуточных данных, если надо, как ты и сказал)
                    Ответить
            • >поспорим
              что тебе дам в ебло я
              Ответить
    • Я вот посмотрел... И думаю... Может у меня где изъян в рассуждениях?

      auto_ptr захватил указатель на что-то, что передано аргументом. Есть, соответственно, предположение, что оно уже указывало на некоторую выделенную память (хотя это не есть обязательно, возможно просто передан указатель, а память динамически выделена внутри функции, правда тогда не ясно к чему release, если память внутри выделена). Теперь представим себе, что функция завершается и очищает свой стек, аварийно, не достигнув освобождения указателя. Тогда будет вызван деструктор auto_ptr , которые освободит память, на которую захваченный указатель направлен был. Это великолепный приём выстрела в ногу. Я создал что-то, потом подсунул в функцию, она аварийно завершилась и вместе с собой отправила в небытие мою память...
      Ответить
      • я уж было подумал, что пойнт никто не прошарит.
        надо было в описании сказать _почему_ я считаю это говнокодом, но я посчитал, что это очевидно. был не прав...
        Ответить
        • Я сейчас честно написал масюсенькую прогу, скомпилил MinGW 3.4 ...
          И... Удаляет собака... Эта функция -- выстрел в ногу без сомнений!
          Ответить
        • Я здесь помню ещё такое было: указатель на обыкновенный C-массив, кладут в auto_ptr, а потом подписывают, что, мол, защита от утечек памяти.

          Этот код заминусовали... Хотя ошибка налицо. Не любят здесь auto_ptr...
          Ответить
          • > Этот код заминусовали... Хотя ошибка налицо.
            не, это, скорее всего, их собственные ошибки, которые они попросту боятся признать или _пока_ не считают их ошибками.
            ничего, как тут уже было сказано - "читайте книжки, они рулез", я лишь добавлю - "поучаствуйте в нескольких крупных проектах, это лучше книжек".
            Ответить
      • Очевидно, exception-safe кода ты пока не писал. И странно учиться этому на подобном сайте.

        По поводу \\\"Я создал что-то, потом подсунул в функцию, она аварийно завершилась и вместе с собой отправила в небытие мою память\\\" посмотри, пожалуйста, http://www.boost.org/doc/libs/1_40_0/libs/ptr_container/doc/tutorial.html, конкретно:

        void zoo::add_animal( animal* a )

        и пример использования:

        the_zoo.add_animal( new mammal(\\\"joe\\\") );

        Как думаешь, куда денется созданный в пользовательском коде mammal joe, если в вектор не удастся запихнуть (the_animals.push_back) указатель?

        Именно это и получится:

        Я создал что-то (new mammal(\\\"joe\\\")), потом подсунул в функцию (the_zoo.add_animal), она аварийно завершилась (the_animals.push_back) и вместе с собой отправила в небытие мою память.
        Ответить
        • >Я создал что-то (new mammal(\\\\\\\"joe\\\\\\\")), потом подсунул в функцию (the_zoo.add_animal), она аварийно завершилась (the_animals.push_back) и вместе с собой отправила в небытие мою память.

          Да. Тут оно, несомненно, уничтожилось к месту. И было бы плохо, если бы оно не уничтожилось.
          А если бы я передавал указатель на кусок кровью и пОтом сосчитанных данных, и они отправились бы в интимное путешествие? Это было бы очень плохо.
          Ответить
    • Моё мнение просто.
      Никогда не захватывать внешние ресурсы, они могут разделяться другими участниками программы.
      Ответить
    • авто поинтер не нужен, есть смарты же
      Ответить
      • В 2009 не было.
        Ответить
        • авто всё равно попахивал даже тогда. К примеру, он вроде бы не умел массив (ну потому что удалять массив надо через delete[] видимо) и это затрудняло его использование в обобщенном программировании.

          В ту пору родилась обзывалка "В ПЛЮСАХ ПАМЯТЬЮ УПАРВЛЯЮТ ВРУЧНУЮ", с которой до сих пор иная скриптоблядь носица
          Ответить

    Добавить комментарий