Trade-off with const in legacy code / Форсировать ли const в старом коде

The English version of this post is below.

————————

Битый час сегодня спорили с коллегой по следующему вопросу. Имеем код:

void foo(T* t) {
  bar(t);
}

Проблема в том, что функция bar является legacy-функцией одной из наших старых библиотек, которую сейчас мы поменять не можем, и ее сигнатура: "void bar(T*)", то есть указатель-параметр не const. Но в реальности эта функция не меняет объект, на который указывает ее параметр.

Далее. Наш новый API, частью которого является foo, есть новейшая разработка, и должна быть спроектирована по уму. С точки зрения запланированной ответственности функции foo, она не должна менять объект, на который указывает t.

Я считаю, что код должен выглядеть так:

void foo(const T* t) {
  bar(const_cast<T*>(t));
}

Мои аргументы: так как контракт функции foo говорит, что эта функция не будет менять объект, на который указывает указатель, то этот факт должен быть отражен в API использованием слова const. И не имеет никакого значения, что по какой-то причине реализация этой функции внутри использует старый код, который неграмотно написан. Да, из-за это приходится делать некрасивое приведение типов, снимая const. Но эта некрасивость локализована внутри foo и в целом не оказывает влияния на стройность нового кода. Более того - если в будущем можно будет отказаться от использования старой функции bar, то проблема вообще исчезнет.

А вот контр-аргумент коллеги: может так случиться, что из-за ошибки в bar константный аргумент функции foo, которая по идее не должна менять аргумент, будет таки изменен, и получится крайне неприятный баг. В итого надо сделать аргумент функции foo НЕ const (и приведение будет уже не нужно), тем самым явно показать конечному пользователю нового API, что не стоит вообще рассчитывать на константность параметра функции foo.

Мы так и не договорились, так как у меня был «прогиб» в плане возможного нарушения константности вне зависимости, что там есть красивый const, а в подходе коллеги было не ясно, как объяснить в документации по функции foo почему и как она может менять свой аргумент - сказать, что мол из-за особенностей реализации foo мы не можем гарантировать константность? Получается, что мы проблемой старого кода портим дизайн нового API.

Дилемма.

P.S. Есть еще один изотерический вариант: внутри foo делать глубокое копирование объекта T и уже его передавать по неконстантному указателю в bar. Лично я, если надо выбирать между быстрым, но «плохим» кодом, и медленным, но со стройным дизайном, чаще выбираю второе, так как завтра купленный более быстрый сервер ускорит хороший, но медленный код, но не сделает плохой код более понятным.

————————

Today we argued an hour with a colleague regarding the following code:

void foo(T* t) {
  bar(t);
}

The problem is that the function bar is a part of a legacy library which we cannot refactor right now. The signature of bar is «void bar(T*)». T is not const. But in reality bar never changes an object referenced by t. This is how it was implemented.

But foo is a part of a brand new API, and we want to make nice or clean. The contract of the function foo says that it doesn’t need to change its parameter.

I think the code should be like this:

void foo(const T* t) {
  bar(const_cast<T*>(t));
}

Why? The contact of foo doesn’t require the pointer t to be non-const. We must reflect this in the API by making t const. It doesn’t matter that for some reason a particular implementation of foo is based on the legacy bar function not having const in the argument but never changing it. Yes, we have to use the ugly const_cast but this bad code is nicely isolated inside foo only and doesn’t affect our nice and clean brand new API. Moreover, if we refactor foo at some point and get rid of legacy bar at all, the problem will disappear completely.

Here is a counterargument from my colleague: it may turn out that the function foo can have a bug and accidentally change t even it is declared as const. The solution is to simply keep the argument of foo non-const. In this case we don’t need that cast, we explicitly show to an end user of foo that she should expect its parameter to be const, and eventually we never violate the contact of the function foo.

Eventually we haven’t agreed. My flaw is that const doesn’t really protect from side effects coming from legacy bar and the argument of foo may be changed regardless being const. My friend’s flaw is that it not easy to explain in the documentation how and why the argument of foo may be changed. Just because our particular implementation dictates this? Such approach spreads the drawback of the legacy code to our nice and shiny new code.

Dilemma.

P.S. There is another esoteric approach — to create a temporary deep copy of T inside foo and pass it to bar by non-const pointer. Personally if I have to choose between quick but badly designed code and slow but nicely written code I usually go for the second one. Tomorrow we can buy another faster computer and the slow code will be faster, but that computer will make the bad code better.