Здравствуйте, B0FEE664, Вы писали:
BFE>>>Рад, что вы ждёте конструктивной критики. S>>Для конструктивной нужно, чтобы критик понимал предмет разговора. Так что нет, не жду. BFE>Т.е. вы думаете, что ваш код никто не поймёт?
Я очень надеюсь, что критиковать код начнут после того как поймут, а не до.
BFE>>>Похоже, что image_bytes объявлен как указатель на const, если это так — то это зачем? S>>Да, это const std::byte*, приходящий извне. BFE>Но зачем нарушать контракт про "const"?
Потому, что в FFMPEG нужно иметь неконстантный указатель.
BFE>>>Почему обращение к fresh_frame->data по индексу, а не по итератору? S>>Потому что это Си-ный массив. Имитировать итераторы через вызовы std::begin/end не вижу смысла. BFE>Почему "эмитировать"? То, что в данном случае итератор — это указатель, ничего не меняет.
Потому, что мне здесь "итераторы" в каком-либо виде не нужны. Если вам кажется, что нужны, то, наверное, для этого есть какие-то доводы?
BFE>Это дело вкуса.
Скорее опыта.
BFE>То, что assert-ы не работают в release — опасное заблуждение.
Если assert-ы работают в релизе, то это уже не assert-ы.
S>>Во-вторых, это гарантируется в принципе Upd: гарантируется, что (std::size(fresh_frame->data) >= std::size(data_plane_sizes)). By design, так сказать. BFE>Если By design, то где static_assert?
В Караганде, вероятно. Потому что by design. Т.к. если в FFMPEG внезапно сделают так, что в AVFrame::data будет меньше элементов, чем в аргументе linesizes для av_image_fill_plane_sizes, то нужно будет что-то менять в консерватории. Может быть законченный параноик будет напишет (не один) static_assert чтобы защититься от такой гипотетической ситуации, но я подобной паранойей еще не страдаю.
Вот по поводу Си-шных кастов паранойя есть, т.к. там опасности не такие гипотетические.
BFE>>>Зачем столько вызовов ptr_cast? Не проще ли завести один указатель? S>>Потому что исходный указатель, image_bytes, постоянно изменяется (это даже в приведенном кусочке видно). А по ходу дела его приходится приводить к std::uint8_t* (да, без const, такова специфика используемой внешней библиотеки). Можно было бы, конечно, вести сразу два указателя, image_bytes и второй, который имеет тип std::uint8_t. Но какой смысл дублировать одно и то же? BFE>Чтобы не полагаться на оптимизатор.
Оптимизатор-то здесь причем? Или вы думаете, что на reinterpret_cast+const_cast есть какие-то накладные расходы в run-time?
BFE>>>Уверены, что []( void *, std::uint8_t * ) -> void {} не ведёт к утечке памяти? (>24 байта за вызов) S>>Мне сложно представить откуда здесь может взяться утечка, т.к. этот фрагмент -- это получение указателя на функцию, которая ничего не делает. Данный указатель требуется для av_buffer_create в качестве "деаллокатора". BFE>А то я не вижу!
Раз высказываете такие опасения, значит не видите. Или видите что-то свое.
S>>Но т.к. деаллокатор здесь не нужен, то и функция пуста. BFE>Вот и хотелось бы понять почему деаллокатор не нужен.
Потому что это деаллокатор не для AVBuffer, а для AVBuffer::data.
Вызывается этот деаллокатор здесь (через здесь):
static void buffer_replace(AVBufferRef **dst, AVBufferRef **src)
{
AVBuffer *b;
b = (*dst)->buffer;
if (src) {
**dst = **src;
av_freep(src);
} else
av_freep(dst);
if (atomic_fetch_sub_explicit(&b->refcount, 1, memory_order_acq_rel) == 1) {
/* b->free below might already free the structure containing *b,
* so we have to read the flag now to avoid use-after-free. */int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE);
b->free(b->opaque, b->data);if (free_avbuffer)
av_free(b);
}
}
BFE>Таким образом: вы всё ещё уверены, что здесь нет утечки памяти?
Я ни в чем на 100% не уверен, но в данном случае очень надеюсь.
Здравствуйте, so5team, Вы писали:
S>Я очень надеюсь, что критиковать код начнут после того как поймут, а не до.
Ну уж нет. Код должен читаться, а не являться ребусом.
BFE>>>>Похоже, что image_bytes объявлен как указатель на const, если это так — то это зачем? S>>>Да, это const std::byte*, приходящий извне.
Да, это я не правильный вопрос задал. Попытаюсь ещё раз. Почему извне приходить указатель на константные данные?
S>Потому, что мне здесь "итераторы" в каком-либо виде не нужны. Если вам кажется, что нужны, то, наверное, для этого есть какие-то доводы?
Вот этот цикл:
auto pImgBytes = ptr_cast( image_bytes );
auto pData = std::begin(fresh_frame->data);
for( const auto plane_size : data_plane_sizes )
{
*pData++ = pImgBytes;
pImgBytes += plane_size;
}
BFE>>То, что assert-ы не работают в release — опасное заблуждение. S>Если assert-ы работают в релизе, то это уже не assert-ы.
А что тогда?
S>В Караганде, вероятно. Потому что by design. Т.к. если в FFMPEG внезапно сделают так, что в AVFrame::data будет меньше элементов, чем в аргументе linesizes для av_image_fill_plane_sizes, то нужно будет что-то менять в консерватории. Может быть законченный параноик будет напишет (не один) static_assert чтобы защититься от такой гипотетической ситуации, но я подобной паранойей еще не страдаю.
Вот видите, для понимания вашего кода нужно знать некий внешний контекст. Т.е. гарантии того, что код будет работать лежат где-то ещё, а не в самом коде. В результате код не очень-то хорош.
S>Потому что это деаллокатор не для AVBuffer, а для AVBuffer::data.
Ага. теперь понятно.
Здравствуйте, B0FEE664, Вы писали:
S>>Я очень надеюсь, что критиковать код начнут после того как поймут, а не до. BFE>Ну уж нет. Код должен читаться, а не являться ребусом.
Код должен решать задачу. При этом (но не вместо!) он должен быть понятным, корректным, надежным, сопровождабельным и т.д.
Но код пишется для решения задачи. И так бывает, что для решения задачи нужно понимание особенностей предметной области (хотя бы на уровне знакомства с используемым инструментарием), без которого (без понимания в смысле) читабельность кода -- это очень эфемерное, но спекулятивное понятие.
BFE>>>>>Похоже, что image_bytes объявлен как указатель на const, если это так — то это зачем? S>>>>Да, это const std::byte*, приходящий извне. BFE>Да, это я не правильный вопрос задал. Попытаюсь ещё раз. Почему извне приходить указатель на константные данные?
Объективная реальность. Скажем, какие-то данные берутся из shared-memory, замапленной в память процесса в режиме read-only.
S>>Потому, что мне здесь "итераторы" в каком-либо виде не нужны. Если вам кажется, что нужны, то, наверное, для этого есть какие-то доводы? BFE>Вот этот цикл: BFE>
Можно и так. Только это дело вкуса. Объективных преимуществ ни у одного из способов нет. Процитированный мной код получился в результате эволюции, на предыдущих стадиях которых, вероятно, plane_index был более нужен именно как индекс.
BFE>>>То, что assert-ы не работают в release — опасное заблуждение. S>>Если assert-ы работают в релизе, то это уже не assert-ы. BFE>А что тогда?
Обычные run-time проверки но с самым, пожалуй, жестким способом реакции на проблему.
BFE>Вот видите, для понимания вашего кода нужно знать некий внешний контекст. Т.е. гарантии того, что код будет работать лежат где-то ещё, а не в самом коде. В результате код не очень-то хорош.
См. выше про потребность быть в предметной области.
Здравствуйте, so5team, Вы писали:
S>Объективная реальность. Скажем, какие-то данные берутся из shared-memory, замапленной в память процесса в режиме read-only.
Не, ну так не честно! Я думал речь про нормальную программу, а не про хакерство.
И всё равно не понятно, зачем вызывать cast на каждый доступ.
Здравствуйте, B0FEE664, Вы писали:
S>>Объективная реальность. Скажем, какие-то данные берутся из shared-memory, замапленной в память процесса в режиме read-only.
BFE>Не, ну так не честно! Я думал речь про нормальную программу, а не про хакерство.
Хакерство здесь не при чем. Можно смотреть на вещи так: есть подсистема A, у которой откуда-то в памяти появляются данные. Часть этих данных должна попасть к подсистеме B, но только на чтение, поэтому подсистема A отдает подсистеме B const-указатель. Далее подсистеме B нужно сформировать AVFrame, который бы использовал данные по полученному указателю. Но тут засада, в AVBuffer указатель неконстантный. Отсюда и происхождение const_cast.
При этом параллельно с подсистемой B над теми же самыми данными может работать и какая-нибудь подсистема C или D. И они так же не должны ничего по указателю менять, поэтому и к ним так же пойдет const-указатель.
BFE>И всё равно не понятно, зачем вызывать cast на каждый доступ.
Там же не зря image_bytes постоянно инкрементируется, т.к. его результирующее значение важно и используется впоследствии (за пределами процитированного фрагмента).
Посему выбор просто:
1) либо работать с исходным константным image_bytes, применяя cast только в тех местах, где реально нужен неконстантный указатель;
2) либо создать временный неконстантный указатель из image_bytes, работать с ним, а потом его значение возвращать, но уже в виде константного.
Второй вариант мне нравится сильно меньше, т.к. значительно увеличивается область, в которой живет неконстантный указатель на константные, по сути, данные.
Здравствуйте, so5team, Вы писали:
S>Но тут засада, в AVBuffer указатель неконстантный.
Вот я тормоз!
Я правильно понимаю, что данные на самом деле не меняются, а просто в библиотеке не отличают константный буфер от неконстантного?
Здравствуйте, B0FEE664, Вы писали:
S>>Но тут засада, в AVBuffer указатель неконстантный. BFE>Вот я тормоз! BFE>Я правильно понимаю, что данные на самом деле не меняются, а просто в библиотеке не отличают константный буфер от неконстантного?
Здравствуйте, Marty, Вы писали:
M>Собери свой проект с /Wall /WX, удивись, сколько варнингов [...]
(Внезапно!) все, кто серьезно относятся к качеству кода с дня нумер один разработки (а не постфактум, да ради прикола), уже собирают со всеми флагами "хорошего тона", и никаких удивлений у них нет.
Иногда еще даже с -Werror впридачу.
Здравствуйте, Marty, Вы писали:
M>Почему сишным кастом пользоваться плохо?
Потому что он умеет втихаря снимать константность и реинтерпретировать. Это позволяет на ровном месте устроить ад и израиль.
Тогда как плюсовые касты явно выражают намерение.
M>Например, я убираю возможную знаковость у char переменной и конвертирую её в unsigned как-то так: M>
M>auto u = (unsigned)(std::uint8_t)ch;
M>
M>Но можно и так: M>
M>auto u = static_cast<unsigned>(static_cast<std::uint8_t>(ch));
M>
Но можно и так
constexpr unsigned unsigned_from_char(char c) { return static_cast<unsigned>(static_cast<unsigned char>(c)); }
constexpr unsigned unsigned_from_char(auto) = delete; // во избежание неявных кастов
...
auto u = unsigned_from_char(ch);
...
M>Во втором случае выражение гораздо менее наглядно, приходится скрипеть мозгами, чтобы понять, что там происходит.
А в третьем случае вызов наглядный, а вся машинерия спрятана под капот, — не приходится скрипеть мозгами, зачем обязательно надо делать два каста, а не сразу единственный (unsigned)ch.
M>Да, для удобства поиска использую именно сишный каст, а не constructor style cast вида int(...)
M>ОбсудиПохоливарим?
Придёт ПМ, включит линтер и-или
-Werror=old-style-cast
(или как оно там в комстроке...) — и холиварь с начальством сколько влезет
Здравствуйте, Marty, Вы писали:
M>Собери свой проект с /Wall /WX, удивись, сколько варнингов будет в простейших битовых выражениях, расставь там плюсовые касты и попробуй это прочитать
Я последние несколько гигантских проектов собирал (в том числе форк хромиума и форк Robotic OS) исключительно с параноидными настройками компилятора.
И ничего.
Сложно первые полгода, пока всё говнище от старых дореформенных разработчиков не вычистишь. Естетственно, не в одно лицо, там трудозатрат на десятки человеко-лет.
А потом — просто не говнокодишь; а линтер и тесты, (а в некоторых случаях — живые ревьюверы) тебе помогают-заставляют.
Здравствуйте, Marty, Вы писали:
M>И это всё как-то работает
Ты же понимаешь, что это вообще не аргумент в разговоре о языковых средствах, которые позволяют уменьшить вероятность ошибки.