Comments 11
Эх, вообще вот это странное поведение realloc из той же серии что и break в switch'ах.
Когда-то может это и было эффективно с точки зрения реализации API… когда «получили null? завершаем программу совсем» — то есть утечки не было, ибо вся память очищалась автоматически выходом.
На текущий момент — это мина замедленного действия.
Реально почти никогда не взрывается (ибо память чтоб закончилась включая оверкоммит — еще постараться надо), зато когда уж взорвётся — всегда очень больно.
Когда-то может это и было эффективно с точки зрения реализации API… когда «получили null? завершаем программу совсем» — то есть утечки не было, ибо вся память очищалась автоматически выходом.
На текущий момент — это мина замедленного действия.
Реально почти никогда не взрывается (ибо память чтоб закончилась включая оверкоммит — еще постараться надо), зато когда уж взорвётся — всегда очень больно.
За рекомендацию №1, вообще-то, нужно бить по рукам. Поскольку это лечение симптомов, а не болезни. Нормальная рекомендация должна включать в себя изменение сигнатур методов CreateSpaceSeparated и ToCSSValue. На вот такие:
Тогда сама сигнатура метода явно указывала бы пользователю, что возвращается владеющий указатель и что вызывающая сторона несет ответственность за его удаление.
То, что предлагаете вы — это заткнуть дырку в одном месте, но оставить ее во всех других местах, где могут вызываться CreateSpaceSeparated и ToCSSValue.
static unique_ptr<CSSValueList> CreateSpaceSeparated() {...}
unique_ptr<const CSSValue> CSSTransformValue::ToCSSValue(....) const {...}Тогда сама сигнатура метода явно указывала бы пользователю, что возвращается владеющий указатель и что вызывающая сторона несет ответственность за его удаление.
То, что предлагаете вы — это заткнуть дырку в одном месте, но оставить ее во всех других местах, где могут вызываться CreateSpaceSeparated и ToCSSValue.
С замечанием согласен.
Тем не менее, в вашем варианте рекомендация может быть ошибочно понята как «перепишите весь проект на умные указатели», после чего отброшена потому что времени на это нет.
В исходном виде рекомендация ценна еще и тем, что показывает как можно переводить на умные указатели слой за слоем, метод за методом.
В исходном виде рекомендация ценна еще и тем, что показывает как можно переводить на умные указатели слой за слоем, метод за методом.
«перепишите весь проект на умные указатели»Начать нужно с того, что unique_ptr — это всего лишь доведенный до ума старый auto_ptr. Говнокод, который не использовал auto_ptr раньше и не использует unique_ptr сейчас, нужно переписывать. Есть на это время или нет — это уже вопрос приоритетов: хотят ли разработчики иметь нормальный код без утечек или их устраивают потенциальные дыры в их коде.
можно переводить на умные указатели слой за слоем, метод за методомМожно сперва поменять сигнатуру CreateSpaceSeparated. Это уже устранит кучу проблем везде, где используется CreateSpaceSeparated. А не только внутри ToCSSValue. Затем можно поменять и сигнатуру ToCSSValue. Вот вы и получите постепенный перевод, только с меньшим количеством проблем.
Единственный разумный довод в пользу рекомендации Андрея Карпова может заключаться в том, что изменение сигнатур может поломать ABI. Но это не имеет отношения к «постепенному переводу». И не исправляет исходную проблему — возврат голых владеющих указателей.
Нужно, нужно его переписывать, не спорю. Вот только объясните менеджерам сначала что следующий релиз откладывается на два месяца потому что тут какие-то голые указатели обнаружились, и новые фичи в этот релиз не войдут…
И еще не забывайте про конфликты в гите. Никому не хочется весь день разгребать ошибки слияния только потому что десять программистов решили переписать один и тот же кусок кода.
Если кому-то дали задачу написать или исправить ToCSSValue — значит, надо исправлять ToCSSValue, а не API сторонних функций.
И еще не забывайте про конфликты в гите. Никому не хочется весь день разгребать ошибки слияния только потому что десять программистов решили переписать один и тот же кусок кода.
Если кому-то дали задачу написать или исправить ToCSSValue — значит, надо исправлять ToCSSValue, а не API сторонних функций.
Вы уж определитесь, вы хотите поговорить о технических проблемах или социальных? Проблемы с менеджерами и конфликтами в git-е — это социальные проблемы организации процесса разработки. К которым можно еще и приплести, например, хз какой code style, в котором разрешено возвращать голые владеющие указатели. Или, скажем, отсутствие должного code review. Или неиспользование статических анализаторов.
А вот наличие API, которое чревато утечками памяти из-за того, что возвращаются голые владеющие указатели — это техническая проблема. И хорошее ее решение было указано.
Так вот, если вы хотите обсуждать социальные проблемы — то это не ко мне.
А вот наличие API, которое чревато утечками памяти из-за того, что возвращаются голые владеющие указатели — это техническая проблема. И хорошее ее решение было указано.
Так вот, если вы хотите обсуждать социальные проблемы — то это не ко мне.
Я говорю о решении технических проблем с учетом возможных социальных.
Ключевое слово «возможных».
Есть рекомендация Карпова, которую сложно назвать удовлетворительной, т.к. она не решает исходной проблемы. И которая сама чревата другими возможными проблемами. Например:
И есть другая рекомендация, которая решает и исходную проблему, и препятствует возникновению других ошибок.
И вот вы, весь из себя такой опытный в социльных проблемах разработки, приходите и говорите: «а ведь вашу рекомендацию можно неправильно понять, тогда как...» При этом вы сами не знаете, есть ли возможность поменять сигнатуры нескольких методов или нет.
Ну, OK. Если вы работаете в условиях, когда вам выдали таск пофиксить утечку памяти в ToCSSValue и ничего больше вы не можете (да и не хотите) сделать, то удовлетворяйтесь дальше своим знанием социальных проблем.
Только это не отменяет неудовлетворительности первоначальной рекомендации господина Карпова.
Есть рекомендация Карпова, которую сложно назвать удовлетворительной, т.к. она не решает исходной проблемы. И которая сама чревата другими возможными проблемами. Например:
const CSSValue* CSSTransformValue::ToCSSValue(....) const {
unique_ptr<CSSValueList> transform_css_value(
CSSValueList::CreateSpaceSeparated());
for (size_t i = 0; i < transform_components_.size(); i++) {
const CSSValue* component =
transform_components_[i]->ToCSSValue(secure_context_mode);
if (!component)
return nullptr;
transform_css_value->Append(*component);
}
return transform_css_value.get();
}И есть другая рекомендация, которая решает и исходную проблему, и препятствует возникновению других ошибок.
И вот вы, весь из себя такой опытный в социльных проблемах разработки, приходите и говорите: «а ведь вашу рекомендацию можно неправильно понять, тогда как...» При этом вы сами не знаете, есть ли возможность поменять сигнатуры нескольких методов или нет.
Ну, OK. Если вы работаете в условиях, когда вам выдали таск пофиксить утечку памяти в ToCSSValue и ничего больше вы не можете (да и не хотите) сделать, то удовлетворяйтесь дальше своим знанием социальных проблем.
Только это не отменяет неудовлетворительности первоначальной рекомендации господина Карпова.
UFO just landed and posted this here
Анализатор не может догадаться про такие вещи. Надо просто выключить диагностику про утечку памяти. Вообще про такое никто никогда ничего нам из пользователей не писал, так что мы просто не думали про такую ситуацию. Будет пользователь — доработаем для него поведение при наследовании от определённых классов.
Sign up to leave a comment.
Chromium: утечки памяти