Как стать автором
Поиск
Написать публикацию
Обновить

Комментарии 35

С одной стороны, разработчикам нужно было проявить твёрдость и просто оставить комментарий "здесь нет ошибки". А для упоротых впихнуть прагмы, пущай спят спокойно.
С другой стороны, есть системы, для которых файлы даже в long не влезут: те же AVR упрутся в 31 бит при работе с SD-карточками. Я понимаю, это особый случай. Но так про особые речь и идёт, всегда будут исключения, которые не удастся покрыть.
Всего голосов 4: ↑4 и ↓0+4
В целом согласен, что предупреждение о сравнении signed/unsigned хитрое: на первый взгляд безобидно, на второй — может скрывать в себе подобные ужасы.
Однако не понимаю, как компилятор должен такое ловить. Следить за любой переменной, в которую присвоили результат вызова ftell()? Сравнение с указателями здесь не слишком подходит — проверить на NULL проще.

На мой взгляд, в данном случае проблема в API. Зачем возвращать long? Хотел было предложить использовать stat(), но stat.st_size тоже обычно signed (off_t). Хотел предложить использовать C++ ifstream.tellg() — он, оказывается, может использовать C API под капотом. Когда казалось бы обыденная операция вызывает столько боли — что-то явно не так.
Вы, кстати, в курсе, что делать fseek() по бинарному файлу — UB? Понимаю, что библиотека работает с текстом, но факт пугающий.
Наверное, самым правильным решением здесь будет mmap() вместо всех этих плясок с буферами.
Вот недавняя статья на ту же тему: http://cpp.indi.frih.net/blog/2014/09/how-to-read-an-entire-file-into-memory-in-cpp/. Кое-какую информацию оттуда взял.
Всего голосов 2: ↑2 и ↓0+2
Вы, кстати, в курсе, что делать fseek() по бинарному файлу — UB?
Почему же? Что-то не могу найти такого в документации…
Всего голосов 2: ↑2 и ↓0+2
Тот самый случай, когда "не в лотерею, а в карты, и не выиграл, а проиграл."

  1. Неопределенное поведение там не в смысле стандарта, а в смысле "непонятно, что будет, иногда работает, иногда нет".
  2. Наблюдается такое поведение не у бинарных, а у текстовых файлов.
    The ftell function obtains the current value of the file position indicator for the stream pointed to by stream. For a binary stream, the value is the number of characters from the beginning of the file. For a text stream, its file position indicator contains unspecified information, usable by the fseek function for returning the file position indicator for the stream to its position at the time of the ftell call; the difference between two such return values is not necessarily a meaningful measure of the number of characters written or read.
Всего голосов 4: ↑4 и ↓0+4
В ссылке из моего комментария цитата из стандарта.

Setting the file position indicator to end-of-file, as with fseek(file, 0, SEEK_END), has undefined behavior for a binary stream (because of possible trailing null characters) or for any stream with state-dependent encoding that does not assuredly end in the initial shift state.

(2011 C standard draft (N1570) §7.21.3 “Files”, footnote to paragraph 9 (footnote 268). Similar wording exists in the 1999 standard, §7.19.3.)

Also: http://stackoverflow.com/questions/21050603/understanding-undefined-behavior-for-a-binary-stream-using-fseekfile-0-seek-e

Это не значит, что каждый с этим столкнётся, но это UB.
Комментарий пока не оценивали0
Просто у вас слишком категорический имератив вышел:

Вы, кстати, в курсе, что делать fseek() по бинарному файлу — UB?

Делать fseek на результат ftell можно в любом случае, а вот на SEEK_END — уже UB, У вас же получается, что там UB во всех случаях.
Всего голосов 2: ↑2 и ↓0+2
А, здесь согласен, нечётко выразился.
Всего голосов 2: ↑1 и ↓10
Компилятор все правильно сделал — указал на весьма нетривиальное место. Тот кто писал этот код, явно не думал о всех случаях соотношения размеров size_t и long. Собственно настоящую ошибку с обработкой больших файлов, программисты так и не исправили. Правильно в этом месте использовать fseeko и ftello, а коммиты с fseek и ftell вообще не должны попадать в репозиторий.
Всего голосов 18: ↑17 и ↓1+16
Изначально проверка была написана с учетом возможно разных размеров size_t и long — оба значения по умолчанию были бы неявно приведены к одному и тому же беззнаковому типу достаточного размера.
Всего голосов 3: ↑2 и ↓1+1
НЛО прилетело и опубликовало эту надпись здесь
Компилятор-то правильно указывает на то, что код кривой. Если уж автор предполагает, что размер файла может вылезти за size_t, то почему он не интересуется, как будет себя вести стек, если в нем выделить соответствующий кусок памяти?

Вторая ,, кривизна'' — это использование fseek/ftell вместо fseeko/ftello.
Всего голосов 6: ↑3 и ↓30
А откуда видно, что память выделяется на стеке? В коде явно написано:

    const size_t size = filelength;
    TIXMLASSERT( _charBuffer == 0 );
    _charBuffer = new char[size+1];

И проблема разработчиков — в том, что, несмотря на использование , они не использовали шаблон std::numeric_limits, который прямо описан, как определённый для арифметических типов и типов, которые через них определены (в том числе и size_t). Это, конечно, в дополнение к использованию fseek вообще.
Всего голосов 3: ↑3 и ↓0+3
Да, new проглядел. Чаще пользую C.
Всего голосов 5: ↑1 и ↓4-3
При компиляции с -std=c++11 clang может выдавать -Wtautological-constant-out-of-range-compare даже при сравнении с std::numeric_limits<size_t>::max()
Всего голосов 1: ↑1 и ↓0+1
Интересно, что бы сказали компиляторы на код
if( (unsigned long)filelength >= min((size_t)-1,(unsigned long)-1) ) {
При каких соотношениях типов будет предупреждение?
Комментарий пока не оценивали0
Ну, да, в местах объединения С кода с С++ могут возникать сложности. Но все же они гораздо меньшего масштаба, чем при объединении кода других различных языков программирования. Тем более, что все решается стандартным путем

f (static_cast<std::common_type<decltype(x), std::size_t>::type>(x) >= std::numeric_limits<std::size_t>::max()) {
Всего голосов 4: ↑4 и ↓0+4
В статье вроде как сказано, что C++11 для расширения аудитории, стараются не использовать. А вот что стало с памятью 
_charBuffer
после срабатывания 
if( read != size )
— вот это интересно. Код не смотрел на гитхабе.
Комментарий пока не оценивали0
Ну сейчас все-таки 2016 год, С++11 вышел уже 5 лет назад; и gcc 4.4 этот код уже поддерживает
Комментарий пока не оценивали0
Боже мой, что это за издевательство над здравым смыслом! Шаблоны-маблоны.

typedef unsigned long filesize_t; // логично, что размер файла не может быть отрицательным?

const size_t max_memory_size = (size_t)-1;
const filesize_t max_memoized_filesize = (filesize_t) max_memory_size;
   // сделаем явный каст - показывая, что можно как срезать, так и расширить

filesize_t filelength;
if (filelength < max_memoized_filesize) { ... }

// ну или всю эту дребедень в одну строку! если уж хочется, чтобы коллеги каждый раз чесали репу
if ((unsigned long)filelength < (unsigned long)(size_t)-1) { ... }
Всего голосов 3: ↑2 и ↓1+1
Плохо, что это в рантайме делается, а хотелось бы в compile time разрулить ситуацию.
Всего голосов 1: ↑0 и ↓1-1
Что делается в рантайме и надо перенести в компиляцию?!
Фактический размер файла?
Или -1 превращается в filesize_t в рантайме?
Всего голосов 2: ↑2 и ↓0+2
Во-первых, все замечательно решается без шаблонов и предупреждений:
if (filelength & ~(unsigned long)(size_t)-1) {
/* error fillength is too large */
}


Во-вторых, считается хорошим тоном нужность подобных проверок определять в configure time (когда запускается cmake или иной аналог configure-скриптов), соответственно в итоге мы никогда не получим «condition always true» для совпадающих по размеру unsigned long и size_t.

В-третьих, если почитать стандарт, то можно написать намного понятнее и также без предупреждений (gcc-4.8.5):
#include <limits.h>

if (filelength > SIZE_MAX) {
/* file too large */
}
Всего голосов 8: ↑8 и ↓0+8
НЛО прилетело и опубликовало эту надпись здесь
Для третьего способа нужно включать stdint.h, и на такую проверку clang тоже может выдавать -Wtautological-constant-out-of-range-compare
Всего голосов 1: ↑1 и ↓0+1
НЛО прилетело и опубликовало эту надпись здесь
при написании кода на obj-c в Xcode — лично встречается 2 случая, когда рекомендуется мутить компилятор clang прагмами.

В первом случае — когда объекту базового класса (вообще-то динамически типизированный язык) пытаюсь передать селектор (нечто вроде указателя на функцию) конкретного класса — он выдает варнинг, что не факт, что метод с таким селектором будет у конечного объекта. Приходится мутить, потому-что перед этим обычно выполняется ручная проверка на то, есть ли данный селектор в таблице селектора объекта (таблица указателей методов). И компилятор не понимает эту проверку!

И еще довольно специфичная работа с памятью в iOS (что-то типа умных указателей со счетчиком ссылок), которые живут в retain-циклах (retain увеличивает число ссылок на 1). Существуют случаи, когда приходится мутить сообщения комилятора, связанные с его анализом этих циклов (особенно при использовании блоков). Блок — это что-то вроде функции, определенной внутри другой функции (функции второго порядка). Причем данного рода прагмы я встречал при анализе кода более, чем в нескольких разных фреймворках

Так и живем (по поводу торжества компилятора на наших костях) ...
Всего голосов 3: ↑2 и ↓1+1
Верните gcc 2.95 !

И да, в c/c++ сильно не хватает @suppresswarnings из java
Всего голосов 1: ↑1 и ↓0+1
Перед выделением памяти стоит проверить, поместится ли требуемый размер в size_t, чтобы избежать труднодиагностируемых логических ошибок при дальшейшей работе.

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

template <typename T1, typename T2>
static bool Fits(T1 &, T2 &)
{
    return sizeof(T1) >= sizeof(T2);
}

const size_t size = filelength;
if( ! Fits(size, filelength) ) {
    return FILE_READ_ERROR;
}
Всего голосов 4: ↑3 и ↓1+2
Хорошо, возьмем систему, где long восемь байт, а size_t четыре байта (например, при компиляции gcc под 32-разрядную систему). Размер файла, допустим, пять килобайт. Предлагаемая вами функция Fits() вернет "false", хотя число около пяти тысяч вполне помещается в четырехбайтный size_t.
Всего голосов 2: ↑2 и ↓0+2
О, я не сразу понял масштаб бедствия.
Вы понимаете, насколько ужасен код, если посторонние люди даже с комментариями, не могут понять, что он должен делать? :)
А в этом случае вас не будут смущать ворнинги типа "преобразование к меньшему типу" в этой строке?

const size_t size = filelength;

Но хорошо, так еще проще.

    const size_t size = filelength;
    if ((long)size != filelength ) {
        return FILE_READ_ERROR;
    }

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

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

    const size_t size = filelength;
    if ((long)size != filelength || (size + 1) == 0) ) {
        return FILE_READ_ERROR;
    }
Всего голосов 1: ↑1 и ↓0+1
Предлагаете явное приведение беззнакового типа к знаковому. При преобразовании беззнакового типа в знаковый, если исходное значение не может быть представлено в результирующем типе, результат преобразования зависит от реализации (implementation-defined). Вы уверены, что стоит полагаться на зависящее от реализации поведение в коде для обработки довольно редких ситуаций, предназначенном для неизвестного заранее множества платформ и компиляторов?
Всего голосов 1: ↑1 и ↓0+1
Теоретически, исходное значение не может быть представлено, если размер long меньше, чем значение в size_t, т.е. sizeof(size_t) > sizeof(long). Практически, в этом случае (с учетом того, что отрицательные значения в filelength — уже само по себе странно) значение в size_t никогда не может быть больше, чем, исходное значение в long filelength.

3 If the destination type is signed, the value is unchanged if it can be represented in the destination type (and
bit-field width); otherwise, the value is implementation-defined.
Всего голосов 1: ↑1 и ↓0+1
Из личного опыта смелой профессиональной молодости. Был backend-демон, который контролировал единственность запущенного инстанса аж сразу двумя способами — блокировка pid-файла и pid-порта (реализация была сделана вручную, без врапперов). IDEA подсветила обе static-переменные как "can be converted to local variables", а я взял и поддался. Телефонный звонок тимлида застал меня в отпуске и я на втором слове понял, какую дурацкую ошибку я совершил (java gc закрывал эти объекты спустя какое-то время после старта приложения).
Всего голосов 3: ↑3 и ↓0+3
… и снова ошибка на самом деле совсем не там, где ее видит IDE :) Если бы для этих объектов использовался try-with-resources (ну или просто вызывался метод close) — то gc бы такой объект не собрал раньше времени.
Всего голосов 2: ↑2 и ↓0+2
Зарегистрируйтесь на Хабре, чтобы оставить комментарий

Ваш аккаунт

Разделы

Информация

Услуги

Техническая поддержка
© 2006–2024, Habr