Передаю привет разработчикам компании Yandex

    ClickHouse and PVS-Studio

    Приблизительно раз в полгода нам пишет кто-то из сотрудников компании Yandex, интересуется лицензированием PVS-Studio, качает триал и пропадает. Это нормально, мы привыкли к медленным процессам продажи нашего анализатора в крупные компании. Однако, раз представился повод, будет не лишним передать разработчикам Yandex привет и напомнить об инструменте PVS-Studio.

    Честно скажу, что статья получилась во многом случайным образом. Нам уже предлагали проверить ClickHouse, но как-то эта идея забылась. На днях, гуляя по просторам интернета, я вновь встретил упоминание ClickHouse и заинтересовался этим проектом. В этот раз я решил не откладывать и проверить этот проект.

    ClickHouse


    ClickHouse — это столбцовая СУБД для OLAP (online обработки аналитических запросов). ClickHouse был разработан в Яндексе для решения задач Яндекс.Метрики. ClickHouse позволяет выполнять аналитические запросы по обновляемым данным в режиме реального времени. Система линейно масштабируемая и способна работать с триллионами записей и петабайтами данных. В июне 2016-го года ClickHouse был выложен в open-source под лицензией Apache 2.0.


    Анализ проекта с помощью PVS-Studio


    Я проверял исходный код ClickHouse, взятый из репозитория 14 августа 2017 года. Для проверки я использовал beta-версию анализатора PVS-Studio v6.17. К моменту публикации статьи уже состоялся релиз этой версии.

    Из проверки были исключены следующие каталоги:
    • ClickHouse/contrib
    • ClickHouse/libs
    • ClickHouse/build
    • также были исключены различные тесты, например, ClickHouse/dbms/src/Common/tests

    Размер оставшегося исходного кода на языке C++ равен 213 KLOC. При этом, 7.9% строк являются комментариями. Получается, что собственного кода, который был проверен, совсем немного: около 196 KLOC.

    Как видите, проект ClickHouse имеет меленький размер. При этом качество кода однозначно высокое и у меня не получится написать шокирующую статью. Всего анализатор выдал 130 предупреждений (General analysis, High и Medium сообщения).

    Про количество ложных срабатываний дать ответ затрудняюсь. Много предупреждений, которые нельзя формально назвать ложными, но при этом и практической пользы от них нет. Проще всего, это будет пояснить, приведя пример.

    int format_version;
    ....
    if (format_version < 1 || format_version > 4)
      throw Exception("Bad checksums format version: " + ....);
    if (format_version == 1) return false;
    if (format_version == 2) return read_v2(in);
    if (format_version == 3) return read_v3(in);
    if (format_version == 4) return read_v4(in);
    return false;

    Анализатор обращает внимание на то, что если начнётся вычисляться выражение (format_version == 4), то оно будет всегда истинно. Как видите, выше есть проверка, что если значение format_version выходит за пределы [1..4], то генерируется исключение. А оператор return false вообще никогда не выполняется.

    Формально, анализатор прав и непонятно, как обосновать, что это ложное срабатывание. С другой стороны, видно, что этот код корректен и просто написан с «запасом надёжности».

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

    switch(format_version)
    {
      case 1: return false;
      case 2: return read_v2(in);
      case 3: return read_v3(in);
      case 4: return read_v4(in);
      default: 
        throw Exception("Bad checksums format version: " + ....);
    }

    Ещё есть предупреждения, про которые я просто не могу сказать: указывают они на ошибку или нет. Я не знаком с проектом и понятия не имею, как должны работать некоторые фрагменты кода. Рассмотрим такой случай.

    Есть некая область видимости с 3 функциями:

    namespace CurrentMemoryTracker
    {
        void alloc(Int64 size);
        void realloc(Int64 old_size, Int64 new_size);
        void free(Int64 size);
    }

    Названия формальных аргументов функций подсказывают, что функции должны принимать на вход какие-то размеры. Подозрение у анализатора вызывают случаи, когда, например, в функцию alloc передаётся не размер структуры, а размер указателя.

    using Large = HyperLogLogCounter<K, Hash, UInt32, DenominatorType>;
    Large * large = nullptr;
    ....
    CurrentMemoryTracker::alloc(sizeof(large));

    Анализатор не знает, ошибка это или нет. Я тоже не знаю, но на мой взгляд, этот код подозрительный.

    В общем, про такие случаи я говорить не буду. Если у разработчиков ClickHouse возникнет интерес, они смогут самостоятельно проверить проект и изучить список предупреждений более подробно. Я же в статье рассмотрю только те фрагменты кода, которые показались мне наиболее интересными.

    Интересные фрагменты кода


    1. CWE-476: NULL Pointer Dereference (3 ошибки)

    bool executeForNullThenElse(....)
    {
      ....
      const ColumnUInt8 * cond_col =
        typeid_cast<const ColumnUInt8 *>(arg_cond.column.get());
      ....
      if (cond_col)
      {
        ....
      }
      else if (cond_const_col)
      {
        ....
      }
      else
        throw Exception(
          "Illegal column " + cond_col->getName() +            // <=
          " of first argument of function " + getName() +
          ". Must be ColumnUInt8 or ColumnConstUInt8.",
          ErrorCodes::ILLEGAL_COLUMN);
      ....
    }

    Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 765

    Здесь неправильно обрабатывается ситуация, когда возникает ошибка. Вместо генерации исключения, произойдёт разыменование нулевого указателя.

    Для создания сообщения об ошибке, происходит вызов функции: cond_col->getName(). Этого делать нельзя, так как указатель cond_col будет нулевым.

    Аналогичная ошибка обнаруживается здесь: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 1061

    Рассмотрим другую вариацию на тему использования нулевого указателя:

    void processHigherOrderFunction(....)
    {
      ....
      const DataTypeExpression * lambda_type =
        typeid_cast<const DataTypeExpression *>(types[i].get());
    
      const DataTypes & lambda_argument_types =
        lambda_type->getArgumentTypes();
    
      if (!lambda_type)
        throw Exception("Logical error: .....",
                        ErrorCodes::LOGICAL_ERROR);
      ....
    }

    Предупреждение PVS-Studio: V595 The 'lambda_type' pointer was utilized before it was verified against nullptr. Check lines: 359, 361. TypeAndConstantInference.cpp 359

    В начале указатель lambda_type разыменовывается, а только затем осуществляется его проверка. Чтобы исправить код, нужно переместить проверку указателя чуть выше:

    if (!lambda_type)
      throw Exception("Logical error: .....",
                      ErrorCodes::LOGICAL_ERROR);
    const DataTypes & lambda_argument_types =
      lambda_type->getArgumentTypes();

    2. CWE-665: Improper Initialization (1 ошибка)

    struct TryResult
    {
      ....
      explicit TryResult(Entry entry_)
          : entry(std::move(entry))        // <=
          , is_usable(true)
          , is_up_to_date(true)
      {
      }
      ....
      Entry entry;
      ....
    }

    V546 Member of a class is initialized by itself: 'entry(entry)'. PoolWithFailoverBase.h 74

    Из-за опечатки, член entry инициализируется сам собой и в результате на самом деле остаётся неинициализированным. Чтобы исправить код, надо добавить подчеркивание:

    : entry(std::move(entry_))

    3. CWE-672: Operation on a Resource after Expiration or Release (1 ошибка)

    using Strings = std::vector<std::string>;
    ....
    int mainEntryClickhousePerformanceTest(int argc, char ** argv)
    {
      ....
      Strings input_files;
      ....
      for (const String filename : input_files)   // <= 
      {
        FS::path file(filename);
    
        if (!FS::exists(file))
          throw DB::Exception(....);
    
        if (FS::is_directory(file))
        {
          input_files.erase(                      // <=
            std::remove(input_files.begin(),      // <=
                        input_files.end(),        // <=
                        filename) ,               // <=
            input_files.end() );                  // <=
    
          getFilesFromDir(file, input_files, recursive);
        }
        else
        {
          if (file.extension().string() != ".xml")
            throw DB::Exception(....);
        }
      }
      ....
    }

    Предупреждение PVS-Studio: V789 Iterators for the 'input_files' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. PerformanceTest.cpp 1471

    Контейнер input_files используется в range-based for loop. При этом, внутри цикла, контейнер может изменяться из-за удаления некоторых элементов. Если читателю не понятно, почему так делать нельзя, предлагаю ознакомиться с описанием диагностики V789.

    4. CWE-563: Assignment to Variable without Use ('Unused Variable') (1 ошибка)

    struct StringRange
    {
      const char * first;
      const char * second;
    
      ....
    
      StringRange(TokenIterator token_begin, TokenIterator token_end)
      {
        if (token_begin == token_end)
        {
          first = token_begin->begin;                // <=
          second = token_begin->begin;               // <=
        }
    
        TokenIterator token_last = token_end;
        --token_last;
    
        first = token_begin->begin;                  // <=
        second = token_last->end;                    // <=
      }
    };

    Анализатор выдаёт два предупреждения:
    • V519 The 'first' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 26, 33. StringRange.h 33
    • V519 The 'second' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 27, 34. StringRange.h 34

    При определённом условии вначале переменной first и переменной second присваивается значение token_begin->begin. Далее значение этих переменных в любом случае вновь изменяется. Скорее всего этот код содержит какую-то логическую ошибку или в нём чего-то не хватает. Например, возможно забыт оператор return:

    if (token_begin == token_end)
    {
      first = token_begin->begin;
      second = token_begin->begin;
      return;
    }

    5. CWE-570: Expression is Always False (2 ошибки)

    DataTypePtr
    getReturnTypeImpl(const DataTypes & arguments) const override
    {
      ....
      if (!((.....))
          || ((left_is_string || left_is_fixed_string) && (.....))
          || (left_is_date && right_is_date)
          || (left_is_date && right_is_string)
          || (left_is_string && right_is_date)
          || (left_is_date_time && right_is_date_time)         // 1
          || (left_is_date_time && right_is_string)            // 1
          || (left_is_string && right_is_date_time)            // 1
          || (left_is_date_time && right_is_date_time)         // 2
          || (left_is_date_time && right_is_string)            // 2
          || (left_is_string && right_is_date_time)            // 2
          || (left_is_uuid && right_is_uuid)
          || (left_is_uuid && right_is_string)
          || (left_is_string && right_is_uuid)
          || (left_is_enum && right_is_enum && .....)
          || (left_is_enum && right_is_string)
          || (left_is_string && right_is_enum)
          || (left_tuple && right_tuple && .....)
          || (arguments[0]->equals(*arguments[1]))))
          throw Exception(....);
      ....
    }

    В этом условии три подвыражения повторяются дважды. Предупреждения PVS-Studio:
    • V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_date_time && right_is_date_time)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
    • V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_date_time && right_is_string)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
    • V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_string && right_is_date_time)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057

    Возможны два варианта. Первый — ошибки нет, условие просто избыточное и его можно упростить. Второй — здесь ошибка и какие-то условия не проверяются. В любом случае, авторам стоит проверить этот фрагмент кода.

    Рассмотрим ещё один случай, когда условие всегда ложно.

    static void ipv6_scan(const char *  src, unsigned char * dst)
    {
      ....
      uint16_t val{};
      unsigned char * colonp = nullptr;
    
      while (const auto ch = *src++)
      {
        const auto num = unhex(ch);
    
        if (num != -1)
        {
          val <<= 4;
          val |= num;
          if (val > 0xffffu)         // <=
            return clear_dst();
    
          saw_xdigit = 1;
          continue;
        }
        ....
    }

    Предупреждение PVS-Studio: V547 Expression 'val > 0xffffu' is always false. The value range of unsigned short type: [0, 65535]. FunctionsCoding.h 339

    При парсинге строки, содержащей IPv6 адрес, некоторые некорректные IPv6 адреса будут восприняты как правильные. Ожидается, что между разделителями могут быть записаны числа в шестнадцатеричном формате со значением не более FFFF. Если число больше, то адрес должен считаться некорректным. Для выявления этой ситуации в коде имеется проверка "if (val > 0xffffu)". Но она не работает. Переменная val имеет тип uint16_t, а значит не может быть больше 0xFFFF. В результате, функция будут «проглатывать» некорректные адреса. В качестве очередной части адреса будут выступать 4 последние 16-ричные числа до разделителя.

    6. CWE-571. Expression is Always True (1 ошибка)

    static void formatIP(UInt32 ip, char *& out)
    {
      char * begin = out;
      for (auto i = 0; i < 3; ++i)
        *(out++) = 'x';
    
      for (size_t offset = 8; offset <= 24; offset += 8)
      {
        if (offset > 0)                     // <=
          *(out++) = '.';
    
        /// Get the next byte.
        UInt32 value = (ip >> offset) & static_cast<UInt32>(255);
    
        /// Faster than sprintf.
        if (value == 0)
        {
          *(out++) = '0';
        }
        else
        {
          while (value > 0)
          {
            *(out++) = '0' + value % 10;
            value /= 10;
          }
        }
      }
      /// And reverse.
      std::reverse(begin, out);
      *(out++) = '\0';
    }

    Предупреждение PVS-Studio: V547 Expression 'offset > 0' is always true. FunctionsCoding.h 649

    Условие "offset > 0" выполняется всегда, поэтому всегда добавляется точка. Мне кажется, ошибки здесь нет и проверка просто лишняя. Хотя конечно я не уверен. Если это всё-таки не ошибка, то проверку следует удалить, чтобы она не смущала других программистов и статические анализаторы кода.

    Заключение


    Возможно, разработчики проекта смогут найти ещё ряд ошибок, просматривая предупреждения анализатора, которые не нашли отражения в статье. Я же закончу повествование, тем более что для того чтобы «передать привет», мне хватило материала :).

    В целом хочу отметить высокое качество кода разработчиков проекта ClickHouse. Впрочем, какими высококлассными не были бы разработчики, они не застрахованы от ошибок, и данная статья вновь это демонстрирует. Статический анализатор кода PVS-Studio поможет предотвратить многие ошибки. Наибольший эффект от статического анализа разработчики получают при написании нового кода. Нет смысла тратить время на отладку ошибок, которые можно обнаружить анализатором сразу после проверки нового кода.

    Приглашаю всех скачать и попробовать PVS-Studio.


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Give my Best Regards to Yandex Developers

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio 375,70
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Комментарии 94
    • 0
      Для создания сообщения об ошибке, происходит вызов функции: cond_col->getName(). Этого делать нельзя, так как указатель cond_col будет нулевым.

      Он возможно будет нулевым, но не всегда из-за наличия второго условия. Думаю там просто стоит добавить дополнительную проверку перед выбросом исключения.
      • +10
        Когда выполняется «throw Exception» указатель не возможно нулевой, а точно нулевой. Посмотрите повнимательнее на код.
        • 0

          А если getName — static функция?

          • +1
            class IColumn : private boost::noncopyable
            {
              ....
              /// Name of a Column. It is used in info messages.
              virtual std::string getName() const = 0;
              ....
            };
            
            template <typename T>
            class ColumnVector final : public IColumn
            {
              ....
              std::string getName() const override;
              ....
            };
            
            using ColumnUInt8 = ColumnVector<UInt8>;
            
            • 0
              я не обладаю плюсами, но разве компилятор не выдаст ошибку компиляции при попытке получить статический метод от экземпляра класса, а не от самого класса?
              по моему если код компилируется то getName явно не статичен
              • 0
                #include <iostream>
                #include <string>
                
                class Lol 
                {         
                public:                                                                         
                        static std::string  name() { return "lol"; }
                };        
                
                int main()
                {         
                        Lol *ptr = nullptr;
                
                        std::cout <<  ptr->name() << std::endl;
                
                        return 0;
                } 

                $ g++ -std=c++17 ./testLol.cpp && echo $?
                0
                $ ./a.out
                lol

                • –1
                  #include <iostream>
                  #include <string>
                  
                  class Lol 
                  {         
                  public:                                                                         
                          std::string  name() { return "lol"; }
                  };        
                  
                  int main()
                  {         
                          Lol *ptr = nullptr;
                  
                          std::cout <<  ptr->name() << std::endl;
                  
                          return 0;
                  } 

                  $ g++ -std=c++17 ./testLol.cpp && echo $?
                  0
                  $ ./a.out
                  lol
                • 0
                  я не обладаю плюсами, но разве компилятор не выдаст ошибку компиляции при попытке получить статический метод от экземпляра класса, а не от самого класса?
                  Не выдаст и, что удивительно, экземпляр класса по-прежнему должен существовать для того, чтобы программа гарантированно работала (хотя на конкретных компиляторах и статические и нестатические фенкции могут отрабатывать при вызове для nullptr).
          • +47
            С другой стороны, видно, что этот код корректен и просто написан с «запасом надёжности».

            Как раз наоборот: этот код так и манит, добавляя/удаляя очередную версию формата, исправить одну из проверок, и забыть исправить вторую.

            switch лучше не только тем, что «затыкает» предупреждение, но и «запасом надёжности» в свете будущих изменений кода.
            • +4
              И читается легче, этот код однозначен. В отличие от ifов.
            • 0

              Кажется, в ваших обзорах ещё не попадалась диагностика V789. Редкий вид (с) :)

              • 0
                У этого проекта достаточно свежий и современный код. Такое не очень часто встречается в проверяемых нами проектах. Обычно у проекта уже есть какое-нибудь наследие, и разрабокта продолжается в том же стиле.
                • 0
                  В данном случае, думаю еще влияет то, что диагностика V789 весьма свежая и появилась недавно в PVS-Studio 6.16 (28 июня, 2017).
              • +7
                Что же за цена, если Yandex не может себе позволить купить себе анализатор?
                • +4
                  Быть может, просто не хотят…
                  • –1
                    Ну, не знаю. По идее Яндекс должен обложиться всевозможными анализаторами кода.
                    • –9

                      Все эти анализаторы кроме пользы добавляют гемор в виде замедления билда, ложных срабатываний, информационного шума и ложного чувства безопасности. Для себя я решил что профессионалам чем меньше таких тулзов тем лучше — гемор легко перевешивает пользу. А нубасам такое тоже особо не поможет — им бы учиться писать юнит тесты, а не тратить время на затыкание варнингов. Возможно, в Яндексе думают так же.


                      Хотя если тесты гонять перед релизом, то гемор будет меньше, и можно будет выцедить нормальное соотношение время/ошибки, так что хз.


                      Дисклеймер: пвс я не пробовал.

                      • +31
                        Шикарный комментарий. Спасибо. Я возьму его в рамочку и буду использовать как собирательный образ заблуждений, касающихся статического анализа кода. И дискутировать на эту тему в статьях и конференциях.
                        P.S. Смайлик ставить не буду, так как я совершенно серьезен. И даже напишу на тему этого комментария маленькую статью-ответ.
                        • +10
                          Не забудьте ещё про ваших реальных конкурентов (не де-юре, а де-факто): динамические анализаторы (ASAN, MSAN, TSAN, UBSAN). У них, конечно, есть беда: срабатывают не так быстро и ресурсов требуют больше. Но зато у них есть и преимущество: если уж *SAN чего-то «такое» у вас при фаззинге нашёл, то отбиваться бесполезно — это ошибка, надо чинить. А со статическим анализатором… всякое бывает.

                          Так что вам нужно влезть в нишу между встроенным в clang/gcc/msvc анализатором (бесплатно и всегда в наличии) и *SAN'ом (сложно и дорого, но зато ложных срабатываний — почти нуль, любую ошибку можно сразу в багтрекер заносить). Так ли велика эта ниша?
                          • 0
                            Ну сфера статических анализаторов, как бы не бедствует: SonarSource, Synopsys (бывший Coverity), Gimpel Software, Rogue Wave Software так далее :).

                            И собственно непонятно, прочему речь идёт про «нишу между». clang/gcc/msvc это своего рода статические анализаторы. С этим все просто. Они работают вот так. Мы лучше. С динамическими анализаторами всё тоже просто. Здесь нет конкуренции, а есть взаимное дополнение. В чем-то лучше динамические анализаторы, в чем-то статические. Пример: находим ошибки в Valgrind.
                            • +4
                              С этим все просто.
                              С этим всё совершенно непросто.

                              Они работают вот так. Мы лучше.
                              Ну если бы вы работали хуже, то в вас бы вообще смысла не было. Но тут важно заметить что анализ с помощью clang/gcc/msvc у вас уже есть — всегда, по умолчанию. А вот всё остальное — нужно устанавливать/настраивать отдельно. А если у вас в проекте контрибуторы из кучи других компаний? Сказать им «включите -Wall -Werror» — это одно, а отлавливать ошибки с помощью PVS-Studio (который они и запустить-то не могут!) — совсем другое.

                              С динамическими анализаторами всё тоже просто. Здесь нет конкуренции, а есть взаимное дополнение.
                              Это всё «халва». От неё слаще во рту не становится, опять-таки.

                              Как я уже сказал — если динамический анализатор чего-то нашёл, то «отмазаться» не получится: «на спор» я могу сдеать код, в котором не будет ошибки, но который будет вопить на анализаторе — но на практике я такого ни разу не видел.

                              А у статического анализатора (любого) есть ложные срабатывания. А если ты ещё и локально код проверить не можешь… В общем — так ли велик выигрыш?

                              Пример: находим ошибки в Valgrind.
                              Странный пример. Valgrind под Valgrind'ом запустить нельзя, так что понятно, что в нём самом могут остаться ошибки, которые PVS-Studio может найти. Но это не такому большому количеству народу нужно…
                              • +2

                                Даже со всеми санитайзерами не всегда понятно как воспроизвоидится некий гейзенбаг. Статический анализатор позволяет перебдеть и, таки, не допустить ситуации, которая спровоцирует баг.

                                • +7
                                  Вот Вы вроде и логично пишете (так может подумать сторонний наблюдатель). Но нет.

                                  Статический анализ — это ответ на вопрос «Что ЕЩЕ я могу сделать для того, чтобы улучшить качество кода?». Если компания зарабатывает с помощью своего программного кода, то она стремиться всеми силами улучшить его качество. Качество — это расширяемость, масштабируемость, простота поддержки и др. Так вот, «что ЕЩЕ я могу сделать, чтобы улучшить качество кода?». Внедрить статический анализ! Не надо думать, что это ВМЕСТО чего-то. Вместо тестов, вместо код ревью, вместо динамических анализаторов… Это «что ЕЩЕ».

                                  «А вот в компании XXX нах… не сдался ваш PVS-Studio!» И такое бывает конечно. Далеко не все компании, который пишут программный код, зарабатывают на нем деньги. Например, российская оборонка программный код пишет, но деньги они зарабатывают не на нем. Поэтому в оборонных предприятиях России PVS-Studio не используют.

                                  А компания Epic Games зарабатывает на своем коде — движке Unreal Engine. Поэтому когда встал вопрос «Что ЕЩЕ мы можем сделать, чтобы код Unreal Engine стал качественнее?», они внедрили PVS-Studio.
                                  • –1
                                    Не надо думать, что это ВМЕСТО чего-то. Вместо тестов, вместо код ревью, вместо динамических анализаторов… Это «что ЕЩЕ».
                                    Так не бывает. Если код вы не пишите для участия в каком-либо конкурсе, где качество — критерий успеха, то вы всегда должны выбирать между улучшением качества кода и чем-то ещё. Можно написать ещё пару тестов, а можно — новую фичу дополнить… что выбрать?

                                    Если компания зарабатывает с помощью своего программного кода, то она стремиться всеми силами улучшить его качество.
                                    Вот только «количество ошибок» — далеко не единственный критерий. Могут быть много других. И уменьшения количества ошибок можно достигать разными средствами — можно вложиться в статический анализатор, а можно — в рефакторинг или более подробный style guide. Что принесёт большую отдачу? Результат — сходу не очевиден, вот совсем не очевиден.

                                    Поэтому когда встал вопрос «Что ЕЩЕ мы можем сделать, чтобы код Unreal Engine стал качественнее?», они внедрили PVS-Studio.
                                    А могли бы вместо этого вложиться в фаззинг — но предпочли PVS-Studio. Вопрос — почему и почему Google поступил иначе?
                                    • +1
                                      Так не бывает.

                                      То есть вы ЛИБО пишите тесты, ЛИБО делаете код ревью? И сразу же увольняете того разработчика, который просит сделать код ревью для его тестов? Ведь он тратит время!

                                      Вопрос — почему и почему Google поступил иначе?

                                      Google сам разрабатывает статический анализ. В clang. Мы знаем людей, которые делают это.

                                      • +1
                                        Можно написать ещё пару тестов, а можно — новую фичу дополнить… что выбрать?

                                        То есть вы ЛИБО пишите тесты, ЛИБО делаете код ревью?

                                        Вы намеренно делаете ложный вывод, доводя до абсурда аргумент khim, или просто некому проревьюить ваш комментарий перед публикацией?
                                        • 0
                                          Вы намеренно делаете ложный вывод, доводя до абсурда аргумент khim

                                          Зачем доводить? Аргумент khim «Так не бывает» и так достаточно абсурден.
                                        • +1
                                          То есть вы ЛИБО пишите тесты, ЛИБО делаете код ревью?
                                          Представьте себе. Я не умею одновременно делать и то и другое.

                                          То что я пишу и тесты и делаю код ревью (но в разное время) просто обозначает что и то и другое посчитали достаточно ценной деятельностью — и потому оставили. Если бы выяснилось что написание тестов в 100 раз эффективнее код ревью (или наоборот) — от одной из этих двух вещей отказались бы.

                                          Google сам разрабатывает статический анализ. В clang. Мы знаем людей, которые делают это.
                                          А это-то вообще тут причём? Да, разрабытывает — потому что автоматический, всегда присутствующий, встроенный в компилятор статический анализ — это хорошо. А вот хорошо ли иметь ещё и отдельных анализатор — это большой вопрос.
                                        • 0
                                          Можно написать ещё пару тестов, а можно — новую фичу дополнить… что выбрать?

                                          По сути статанализатор это некая замена (условно, конечно) тем самым паре тестов сверху.
                                          • +5
                                            А могли бы вместо этого вложиться в фаззинг — но предпочли PVS-Studio. Вопрос — почему и почему Google поступил иначе?
                                            Я думаю, это очевидно.

                                            У Гугла приоритет — сетевые сервисы, там ошибка, приводящая к DoS или RCE это очень серьёзно.

                                            Для UE такие ошибки некритичные (если падает при загрузке какой-то кривой модельки, никто и не заметит, потому что модельки загружают туда прямые).

                                            Главное в UE — чтобы графика правильно рисовалась и физика моделировалась. Тут фаззинг бессилен. Ну скормишь рандомные данные в движок, получишь какой-то результат. А как проверить, что результат верный?
                                          • 0
                                            Сертификационные лаборатории активно пользуются статическими (как и динамическими) анализаторами, так что утверждение не совсем корректно.
                                            • 0
                                              Давайте не будем про сертификационные лаборатории? Дай им всем Бог здоровья и процветания!
                                    • 0
                                      Пишу на яве, может у вас лучше, я честно написал. :)

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

                                      В общем, пользуйтесь, рад что повеселил, хоть смайлик и не поставили. :D
                                    • 0
                                      А в юнит тестах не бывает ошибок?
                                      • 0
                                        Бывает и много. Из статей обычно исключаются.

                                        Бывает ещё хуже: как-то исправление настоящей ошибки по нашему отчёту привело к завалу сотни тестов в одном проекте. Отличные тесты были, не правда ли?
                                        • +1
                                          Тесты — это просто проверка соответствия фактического поведения программы ожидаемому. Они вполне могли быть отличными.
                                • +3
                                  Вопрос не в цене анализатора. Вопрос в цене внедрения и поддержки. Ложные срабатывания стоят денег, интеграция стоит времени и т.д. и т.п.

                                  Не думаю что проблема в цене лицензии, Яндекс точно может себе её позволить.
                                  • +5

                                    Ну-ну, написал я им, внедрил в линукс билды по триальному ключу, интегрировал, написал маны для соратников. А когда дошёл разговор до платных лицензий, они &₽?!@"&!/₽-&!₽ Ой, чуть NDA не сдержал.

                                    • 0
                                      То, что у Яндекса есть деньги — вовсе не значит, что они готовы им разбрасываться.

                                      Говорить о цене стоило до всего остального. Ибо вполне может оказаться, что у них, например, в билд-ферме 10000 машин — и что, для каждой лицензию покупать?

                                      Триальная лицензия — это уж точно не для интеграции и для написания манов для сотрудников…
                                      • 0

                                        Да, вы правы. Договариваться стоило на берегу. С другой стороны, хотелось убедить своё начальство аргументами "смотрите, как круто это работает на нашем проекте! Вот нашло! И вот нашло!", но ₽&@?!'-/@&₽):;@/"₽&@₽

                                      • 0
                                        &₽?!@"&!/₽-&!₽


                                        Не смог расшифровать. Мы что-то сделали не так? Как мы можем это исправить?
                                        • 0

                                          Сделайте исключение сорцов per project, а не глобально, с поддержкой раскрытия переменных окружения из VS-проектов для формирования полных путей вместо масок.

                                          • –1
                                            Просим написать нам в support.
                                            • +2

                                              Зачем? Был вопрос, что можно исправить, я написал.

                                          • +1

                                            Я не уверен, что я могу обсуждать денежные вопросы прилюдно без вашего разрешения. Если позволите, я объясню, что вы делаете не так, и откуда берутся "&₽?!@"&!/₽-&!₽".

                                            • –3
                                              Я очень благодарен за любую критику, она позволяет стать лучше. Пожалуйста, напишите свое мнение.
                                              • +2

                                                Знаете, я писал длинное письмо, но потом передумал. Кратко: не предлагайте отделу 20 лицензий, если отдел запрашивает 8. Отделу и 1 на билд сервер хватило бы, но отдел попросил честные 8, т.к. в команде 7 плюсовиков. Нельзя быть таким жадным, Евгений.

                                                • –3
                                                  Наши Team License как раз удовлетворяют вашему требованию. Напишите нам и продолжим общение с этого места. Скорее всего кто-то что-то недопонял или неверно сформулировал запрос. Не важно, с нашей или с вашей стороны.
                                                  • +1

                                                    Да смысла нет.


                                                    Ребята из Я, я вам написал маны по настройке Coverity, извините, но ссылок дать не могу, так как не в компании. Реально хорошо находит баги, умеет делать рассылки и прочие кошерные вещи. И проблем с закупкой нет :)

                                                    • –3
                                                      А, вы просто хотите Coverity в Яндекс продать, а мы вам мешаем, понятно…

                                                      Жесть какая, маны он написал… Вот подробнейшая документация по PVS-Studio, и не надо никакие левые маны брать.
                                                      • +2

                                                        :D ну когда я был в Я и у меня был выбор между вашим продуктом и Coverity, то я сделал выбор не в вашу пользу. Видимо, не зря.


                                                        • проснулась моя девушка и попросила удалить слово недоговно
                                      • 0
                                        А откуда у вас такая информация? Возможно они coverity используют или еще что-нибудь?
                                      • +51
                                        Приблизительно раз в полгода нам пишет кто-то из сотрудников компа
                                        нии Yandex, интересуется лицензированием PVS-Studio, качает триал и пропадает.

                                        Видимо у них в плане работ стоит проверка анализатором каждые полгода.
                                        • +5

                                          Разработчикам PVS-Studio: очень понимаю сотрудников Yandex — с ценовой политикой на продление лицензий вида "а давайте увеличим стоимость продления лицензии PVS-Studio на рандомную существенную величину" уже не первый год подряд и как результат проблематичностью бюджетирования, также начинаем поглядывать на альтернативы.

                                          • –1
                                            Не понимаю о чем речь. Ценовая политика в плане продлений не менялась много лет. 80% от цены новой лицензии.
                                            • +1
                                              А новая цена тоже не менялась много лет?
                                              • –1
                                                Конечно менялась! У нас появился C# анализатор, у нас появился C++ анализатор под Linux, поддержка SonarQube, интеграция с CI и многое другое. Сотни диагностик, сотни страниц документации.

                                                Нет никакой возможности продавать по цене пятиледней давности.

                                                Однако если вы не просто так в комментах пообсуждать, а по делу, то напишите мне и я расскажу о способах справиться с указанной бедой.
                                                • +5
                                                  А почему бы цену продления не делать фиксированной в момент покупки?

                                                  Но я так, просто в комментах пообсуждать. Хоть я бы и очень хотел купить ваш продукт себе и на своей работе — возможности такой нет. Для собственного использования слишком жирно, а на работе сейчас тоже бюджета нет.
                                                  Так что я вас без коммерческого потенциала отвлекаю.
                                                  • –9
                                                    А почему бы цену продления не делать фиксированной в момент покупки?


                                                    Но я так, просто в комментах пообсуждать.


                                                    Извините, Хабр — технический ресурс и если мы будем обсуждать нюансы бизнеса, то это будет не интересно аудитории.
                                                    • +20
                                                      Извините, Хабр — технический ресурс и если мы будем обсуждать нюансы бизнеса, то это будет не интересно аудитории.

                                                      Друг мой, аудитория годами читает Milfgard'a и нюансы бизнеса МосИгры (это как яркий пример, есть еще вернувшийся Мегамозг).

                                                      Возвращаясь же к топ-комментарию:
                                                      Конечно менялась! У нас появился C# анализатор, у нас появился C++ анализатор под Linux, поддержка SonarQube, интеграция с CI и многое другое. Сотни диагностик, сотни страниц документации.

                                                      Эта фраза может произвести двоякое впечатление. Можно возразить что-то вроде "с чего мне платить за SonarQube и С++ анализаторы под Linux? У меня .NET проекты!!!111".
                                                      • +16
                                                        Слушайте, ну мы все взрослые люди и понимаем, что у вас есть определенный бизнес процесс и определенные причины не обсуждать ценовую политику публично.
                                                        Но, попытка это прикрыть интересами сообщества выглядит толстым таким лицемерием и воспринимается крайне негативно.
                                                    • +12
                                                      У нас появился C# анализатор, у нас появился C++ анализатор под Linux, поддержка SonarQube, интеграция с CI и многое другое.

                                                      «С этого года во всех наших автомобилях установлены крылья и ракетные двигатели, при приезде на ТО мы в обязательном порядке их ставим и вы обязаны за них заплатить даже если они вам не нужны».

                                                      Примерно так это звучит. Зачем компании, которая пишет на плюсах под винду версия под Linux и C# анализатор? Сделайте это отдельной лицензируемой функциональностью, кому надо — тот купит.
                                              • +1
                                                а вы код эрланга (виртуальной машины) не проверяли?
                                              • +4
                                                А у вас есть диагностика на использование значения, которое переместили?
                                                Что-то вроде этого:
                                                    std::vector<int> a = ...;
                                                    ...
                                                    auto b = std::move(a);
                                                    ...
                                                    auto s = a.size(); // ???
                                                
                                                • 0
                                                  std::move ничего не перемещает.
                                                  • 0
                                                    Это я к тому, что a остался вполне себе валидным объектом.
                                                    • 0
                                                      Но это только стандартная библиотека гарантирует «valid but unspecified state». Для прочих объектов (согласно стандарту) валидность не требуется.
                                                      • +3
                                                        Валидный останется объект или не валидный — не очень важно.
                                                        Главное, семантика перемещения. Объект а после перемещения хранит непонятно что (если move реализован через swap, то в а лежит содержимое b), и вроде нет никаких причин работать с а, кроме как присвоить ему новое значение.
                                                        Мне кажется это главным изъяном операции перемещения введеной в С++11 — появление объектов с непонятным состоянием, которые по идее должны закончить свою область видимости после перемещения.
                                                  • +1
                                                    Пока нет. Подумаем.
                                                  • 0
                                                    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод

                                                    «Перевод» очень похож на немного изменённый результат google translate.
                                                    Учитывая, что англоязычный сегмент рынка больше русскоязычного, такой подход выглядит странно.

                                                    • –2
                                                      Что делать? Подскажите как улучшить ситуацию.
                                                      • +4

                                                        Выучить английский; нанять того, кто знает английский. Масса вариантов.

                                                  • +16
                                                    Спасибо, анализ полезный! Поправили указанные ошибки:
                                                    github.com/yandex/ClickHouse/pull/1204

                                                    (Одной из ошибок уже не было в master, так как анализировалась более старая версия кода.)
                                                    • +1

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


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


                                                      Штука в том, что профессия разработчика/программиста была, есть и будет инженерной, где требуется квалификация и здравый смысл.
                                                      А ее всячески пытаются запихнуть в ремесленничество с помощью как организационных, так и технических средств.
                                                      Все это нужные вещи, конечно, но делаются не в той последовательности.

                                                      • 0
                                                        хорошо бы писать код так, чтобы компилятор не выдавал стандартных ворнингов (а где вы видели это в больших проектах?).

                                                        Большие проекты — это сколько по-вашему?

                                                        Штука в том, что профессия разработчика/программиста была, есть и будет инженерной, где требуется квалификация и здравый смысл.
                                                        А ее всячески пытаются запихнуть в ремесленничество с помощью как организационных, так и технических средств.

                                                        Точно так же как и автоматическая коробка передач на атомобиле, стиральна машина-автомат и тому подобное. Низводят людишек до сосотояния операторов.
                                                      • +3
                                                        На предыдущей работе писал на чистом «C» среда разработки VisualDSP++, после очередной статьи с вашими хвалебными речами о PVS-Studio решил попробовать протестировать свои проекты. А вот хрен. Не работает PVS-Studio в связке с VisualDSP++. Ну думаю ладно, надо на чем то еще попробовать. Нашел код написанный под AVR опять же на чистом «C». Код писался в «блокноте» и собирался в AVR GCC — то же облом. Ну думаю, дай попробую проекты, которые писал под STM32 на «С» и собирал в IAR. Но и тут какая то лажа.
                                                        • –2
                                                          Вы выкатили список нестандартных тулчейнов. Понятное дело, что у подобного может не быть заявленной поддержки, и Вы испортили впечатление о продукте на пустом месте. Подобную информацию следует изучать в документации и поддержке заранее.
                                                          • +4
                                                            Ок. Идем на сайт PVS-Studio https://www.viva64.com/ru/t/0046/
                                                            Читаем:
                                                            Другие преимущества статического анализа кода:

                                                            2. Статический анализ не зависит от используемого компилятора и среды, в которой будет выполняться скомпилированная программа. Это позволяет находить скрытые ошибки, которые могут проявить себя только через несколько лет. Например, это ошибки неопределенного поведения. Такие ошибки могут проявить себя при смене версии компилятора или при ...
                                                            • –3
                                                              Там объёмный текст о преимуществах и недостатках статического анализа кода как методологии. Вы даже заголовок процитировали. Опрометчивая претензия.
                                                              • +1
                                                                www.viva64.com/ru/pvs-studio
                                                                Быстрый старт в Windows и Linux

                                                                Отметим только, что в PVS-Studio для Windows и Linux предусмотрены специальные утилиты, собирающие информацию о запусках компилятора. Эти инструменты позволяют быстро выполнить анализ проекта, собираемого любым способом. Вы можете быстро познакомиться с возможностями анализатора, не тратя время на его интеграцию с makefile или сборочным скриптом. См. описание утилиты Standalone (Windows) и pvs-studio-analyzer (Linux).

                                                                Это тоже опрометчивая претензия?
                                                                • –4
                                                                  Отчасти. Я же не знаю на сколько давно Вы там работали. Эти механизмы достаточно новые, сайт обновляется. Уже поздно копать в эту строну, но я рад, что Вы всё-таки решили ознакомиться с описанием и документацией.
                                                                  • +1
                                                                    Первый раз пробовал PVS-Studio примерно год назад (на DSP++ и GCC), второй раз (на IAR-е) сегодня. Поэтому и пишу, что ничего не изменилось.
                                                        • 0
                                                          Интересный привет) Что тут сказать)
                                                          • –3
                                                            Интересно, а когда-нибудь проверяли исходники PVS-Studio при помощи самой PVS-Studio?
                                                          • 0
                                                            Интересно, а не было попыток анализировать с помощью PVS-Studio проекты и код на LISP/Clojure/Racket/Scheme/...?
                                                            • –4
                                                              Это не имеет практического смысла.
                                                            • 0
                                                              Спасибо за статью! Скажите, появится ли в PVS-Studio Java?
                                                              • +1
                                                                Но зачем?
                                                                Есть же FindBugs и инспекции в IDEA.
                                                                • 0
                                                                  Это другой вопрос. Для C#/С++/С тоже есть инструменты статического анализа, это не мешает PVS-Studio делать это.
                                                                • 0
                                                                  Появится. Но пока это только в абстрактном плане.

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

                                                                Самое читаемое