Hi, All!

У меня в VisualC 10 есть класс с перегруженным operator==. Когда я в коде пишу сравнение двух экземпляров этого класса, в отладчике я вижу, что в перегруженный operator== выполнение программы просто не заходит, считая результат операции всегда TRUE. Почему?

class CMoney
{
public:
    BOOL operator==(CMoney &other)
    {
        ASSERT(m_iCurrencyID == other.m_iCurrencyID);
        return (m_fAmount == other.m_fAmount);
    }
private:
    double m_fAmount;
    int m_iCurrencyID;
}

Сейчас закомментировал этот оператор, и проект скомпилировался несмотря на то, что в коде есть его вызовы типа if (money1 == money2). Я явно что-то делаю не то, но вот что...

Здравствуйте, sushko, Вы писали:

И хорошо что не заходит. Сравнение даблов через == это как разбрасывание граблей по полю с высокой травой.

Здравствуйте, sushko, Вы писали:

S>Сейчас закомментировал этот оператор, и проект скомпилировался несмотря на то, что в коде есть его вызовы типа if (money1 == money2). Я явно что-то делаю не то, но вот что...
сигнатура метода должна быть такой:

bool operator==(const CMoney& other) const
{
...
}

нужно показать больше кода (минимальный пример) и ошибку компилятора, чтобы мы могли помочь

Здравствуйте, sushko, Вы писали:

S>У меня в VisualC 10 есть класс с перегруженным operator==. Когда я в коде пишу сравнение двух экземпляров этого класса, в отладчике я вижу, что в перегруженный operator== выполнение программы просто не заходит, считая результат операции всегда TRUE. Почему?

Лучше сделать operator== константным, чтобы он работал бы и константными объектами. И добавить оператор != для полноты:

S>

S>class CMoney
S>{
S>public:
S>    bool operator==(CMoney const& other) const
S>    {
S>        ASSERT(m_iCurrencyID == other.m_iCurrencyID);
S>        return (m_fAmount == other.m_fAmount);
S>    }
      bool operator!=(CMoney const& other) const { return !(*this == other); }
S>private:
S>    double m_fAmount;
S>    int m_iCurrencyID;
S>}
S>

Вообще сравнивать на равенство числа в формате с плавающей точностью (float/double) — очень плохая идея, т.к. из-за потери точности результаты математически равных выражений могут оказаться неравными. И название класса как-бы намекает на необходимость точных результатов.

Включив режим телепата, рискну предположить, что CMoney::operator== не вызвается из-за того, что в классе определен оператор преобразвания к какому-то встроенному типу, например CMoney::operator double()

При отладке можно поставить breakpoint на if (money1 == money2) и воспользоваться командой step into которая покажет, что именно там вызывается, или ассемблер поглядеть.
А проблема скорее всего из-за того, что там в классе объявлен какой-нибудь оператор приведения к встроенному типу, который и вызывается неявно перед сравнением.
Ну и, как выше писали, сравнивать double через == не стоит. Это не математика, вычисления одной величины разными способами дает не совсем одинаковый результат. Поэтому при сравнении стоит ввести некоторую величину погрешности, при который две величины будут считаться (примерно) равными.

Здравствуйте, sushko, Вы писали:

Может нужно добавить:

S>

S>    BOOL operator==(const CMoney &other)
S>

Деньги хранить в даблах? Хм...

И да, сам метод тоже константным стоит сделать

Здравствуйте, sushko, Вы писали:

S>Сейчас закомментировал этот оператор, и проект скомпилировался несмотря на то, что в коде есть его вызовы типа if (money1 == money2). Я явно что-то делаю не то, но вот что...

дай угадаю. Сравниваешь не объекты, а указатели?

И еще, добавлю к уже сказанному — выкинь ASSERT. Совсем. И больше не трогай.

S>>Сейчас закомментировал этот оператор, и проект скомпилировался несмотря на то, что в коде есть его вызовы типа if (money1 == money2). Я явно что-то делаю не то, но вот что...
U>сигнатура метода должна быть такой:
U>

U>bool operator==(const CMoney& other) const
U>{
U>...
U>}
U>

Спасибо, помогло!

U>нужно показать больше кода (минимальный пример) и ошибку компилятора, чтобы мы могли помочь

Не было никаких ошибок компилятора, все компилировалось и работало. Правда, работало неправильно

S>>Сейчас закомментировал этот оператор, и проект скомпилировался несмотря на то, что в коде есть его вызовы типа if (money1 == money2). Я явно что-то делаю не то, но вот что...

L>дай угадаю. Сравниваешь не объекты, а указатели?

Не, объекты сравниваю.

L>И еще, добавлю к уже сказанному — выкинь ASSERT. Совсем. И больше не трогай.

Как говорят братья-уголовники — обоснуй?

Здравствуйте, sushko, Вы писали:

А давайте подумаем как лучше задизанийть класс CMoney? Мне кот кажется CurrencyId должен быть классом Currency, а CMoney шаблонным классом и параметризироваться Currency. Тогда это позволить сделать проверку в compile time сравнения "двух денег" разной Currency.

PM>Лучше сделать operator== константным, чтобы он работал бы и константными объектами. И добавить оператор != для полноты:

Вы были правы, и именно в этом все и было дело. Оператор== незаоверрайдился из-за того, что он был не-константным и не совпали сигнатуры методов.

PM>Вообще сравнивать на равенство числа в формате с плавающей точностью (float/double) — очень плохая идея, т.к. из-за потери точности результаты математически равных выражений могут оказаться неравными. И название класса как-бы намекает на необходимость точных результатов.

К сожалению, вылечить это — примерно 3 месяца работы: проект большой, и этот класс не единственный, кто (напрасно) работает с double. Когда-нибудь сделаю.

PM>Включив режим телепата, рискну предположить, что CMoney::operator== не вызвается из-за того, что в классе определен оператор преобразвания к какому-то встроенному типу, например CMoney::operator double()

Берегите ваш режим телепата, он, по ходу, работает очень точно. Это был operator LPCTSTR()

K>А давайте подумаем как лучше задизанийть класс CMoney? Мне кот кажется CurrencyId должен быть классом Currency, а CMoney шаблонным классом и параметризироваться Currency. Тогда это позволить сделать проверку в compile time сравнения "двух денег" разной Currency.

Не, так, к сожалению, сделать не получится. В этом конкретном проекте, я имею в виду.

Здравствуйте, sushko, Вы писали:

L>>дай угадаю. Сравниваешь не объекты, а указатели?
S>Не, объекты сравниваю.

Значит, не угадал.

L>>И еще, добавлю к уже сказанному — выкинь ASSERT. Совсем. И больше не трогай.

S>Как говорят братья-уголовники — обоснуй?

Уголовники мне не братья.

Во-первых, бездумное использование ассертов как минимум бессмысленно, а чаще всего просто вредно. Использовать нужно тесты.
А в данном случае вообще вредно — в дебаге ты получишь вылет при сравнении разных валют, а в релизе такие значения молча прожуются. Салют трудноуловимым багам, и вопрос даже не в "если" а в "когда". Рано или поздно в продакшене у тебя кто-то купит нечто, что стоит 20 килоуе за 20 килонеуе.
Если нельзя сравнивать разные валюты, то это должно быть закреплено контрактом в коде, а не отладочной ловушкой. В данном случае можно бросить исключение. Если логика допускает, то этот оператор может тупо возвращать false при попытке сравнения разных валют. С оператором < такой номер уже так просто не пройдет.

Здравствуйте, landerhigh, Вы писали:

L>Во-первых, бездумное использование ассертов как минимум бессмысленно, а чаще всего просто вредно. Использовать нужно тесты.

Да ладно!
Ассерты нужны как раз на тот случай, чтобы удостовериться в покрытии тестами. А то на всякий чих не наздравствуешься.

Вот намедни поймал такую протечку ошибки: глючный сервер заставил глючить SSL-стек браузера. Теоретически, надо было это диагностировать на ранней стадии, но у авторов не хватило фантазии сделать такие проверки и покрыть такими тестами.
В итоге мусор на входе просочился за гематоэнцефалический барьер и привёл к неопределённому поведению. Которое выразилось в нарушении ассерта, к счастью, не очень далеко от места возникновения ошибки. А не было бы ассерта, полетели бы клочки по закоулочкам.

L>А в данном случае вообще вредно — в дебаге ты получишь вылет при сравнении разных валют, а в релизе такие значения молча прожуются. Салют трудноуловимым багам, и вопрос даже не в "если" а в "когда". Рано или поздно в продакшене у тебя кто-то купит нечто, что стоит 20 килоуе за 20 килонеуе.
L>Если нельзя сравнивать разные валюты, то это должно быть закреплено контрактом в коде, а не отладочной ловушкой. В данном случае можно бросить исключение.

В этом большая польза макросов. Определить ассерт так, чтобы заодно кидал исключение "всё вааащщще плохо!" (и в релизе тоже).
Если клиентский код должен отличаться повышенной живучестью, он это исключение поймает и перезапустит нужную подсистему.

L>Если логика допускает, то этот оператор может тупо возвращать false при попытке сравнения разных валют. С оператором < такой номер уже так просто не пройдет.

Здравствуйте, Кодт, Вы писали:

К>Да ладно!
К>Ассерты нужны как раз на тот случай, чтобы удостовериться в покрытии тестами. А то на всякий чих не наздравствуешься.

Эээ, не догнал от слова совсем. Как ассерты помогут удостовериться, что все сценарии покрыты?
Не говоря уже о том, что тесты вообще нужно в релизе гонять

К>Вот намедни поймал такую протечку ошибки: глючный сервер заставил глючить SSL-стек браузера. Теоретически, надо было это диагностировать на ранней стадии, но у авторов не хватило фантазии сделать такие проверки и покрыть такими тестами.
К>В итоге мусор на входе просочился за гематоэнцефалический барьер и привёл к неопределённому поведению. Которое выразилось в нарушении ассерта, к счастью, не очень далеко от места возникновения ошибки. А не было бы ассерта, полетели бы клочки по закоулочкам.

Это называется "повезло". UB оно ж на то и UB, что его B — U. Оно с тем же успехом могло и через ассерт просочиться и до него дров наломать, и вообще левую память снайперски порасстреливать. И опять же, а если оно только в релизе стреляет?

Вот у меня недавно случай был. На юнит-тестах, кстати. В дебаге все пучком, а в релизе случилось нечто. Тест кейс практически вида ASSERT_EQ(3, code_that_does_something_then_adds_two_numbers(2, 1)) падал, код упорно вычислял 2+1 как 2. На деле немножко сложнее, конечно, но суть именно такая. Оказалось, баг в оптимизаторе, да не простой, а во время link-time code generation. Оно посчитало, что результат сложения 2 и 1 никому будет не нужен и аккуратно выкинуло соотвествующий кусочек логики
И что обидно, урезать проект до минимума, на котором воспроизводится, не удалось. Видимо, связано с объемом кода или еще какой чертовщиной. Вот не будь у меня привычки полагаться на проверку наблюдаемого поведения (ака тесты), был бы мне сюрпризец через месяц, когда прототип в тестирование передавали.

L>>А в данном случае вообще вредно — в дебаге ты получишь вылет при сравнении разных валют, а в релизе такие значения молча прожуются. Салют трудноуловимым багам, и вопрос даже не в "если" а в "когда". Рано или поздно в продакшене у тебя кто-то купит нечто, что стоит 20 килоуе за 20 килонеуе.
L>>Если нельзя сравнивать разные валюты, то это должно быть закреплено контрактом в коде, а не отладочной ловушкой. В данном случае можно бросить исключение.

К>В этом большая польза макросов. Определить ассерт так, чтобы заодно кидал исключение "всё вааащщще плохо!" (и в релизе тоже).

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

Здравствуйте, утпутуук, Вы писали:

S>>

S>>    BOOL operator==(const CMoney &other)
S>>

У>Деньги хранить в даблах? Хм...

Вообще-то допустимо, если это, например, финансовые или статистические расчёты. Там точность не нужна.

Здравствуйте, landerhigh, Вы писали:

L>Как ассерты помогут удостовериться, что все сценарии покрыты?

ты слишком категоричен, польза от ассертов в дебаге есть
для повышения качества софта используют кучу средств, в том числе
1) ассерты только в дебаге
2) ассерты в релизе
3) исключения
4) покрытия тестами
и тд

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

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

Здравствуйте, uzhas, Вы писали:

U>для повышения качества софта используют кучу средств, в том числе
U>1) ассерты только в дебаге

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

U>2) ассерты в релизе

Ой мама

U>3) исключения

Это ортогонально качеству софта

U>4) покрытия тестами
U>и тд

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

Это больше вопрос проектирования и/или документирования. Коду вида "универсальный всемогутор" обкладывание ассертами помогает плохо, код же, при написании которого старались следовать SOLID, зачастую с нарушением контракта еще и не применишь.

U>они отключаются в релизе, что может способствовать созданию более оптимального кода
U>на практике это приводит дилемме: либо надежно, либо быстро

ага, и к тому, что продакшен идет дебаг-сборка

Здравствуйте, landerhigh, Вы писали:

К>>Ассерты нужны как раз на тот случай, чтобы удостовериться в покрытии тестами. А то на всякий чих не наздравствуешься.
L>Эээ, не догнал от слова совсем. Как ассерты помогут удостовериться, что все сценарии покрыты?
L>Не говоря уже о том, что тесты вообще нужно в релизе гонять

А вот так. Сидишь себе в дебажной сборке, отлаживаешь совсем другой предмет... и тут вдруг бабах, пароход перевернулся! Теряешь монокль O_o, вставляешь монокль обратно, и начинаешь увеличивать метрику кода WTF/min.

L>Это называется "повезло". UB оно ж на то и UB, что его B — U. Оно с тем же успехом могло и через ассерт просочиться и до него дров наломать, и вообще левую память снайперски порасстреливать. И опять же, а если оно только в релизе стреляет?

А не было бы ассерта, вообще не повезло бы.

К>>В этом большая польза макросов. Определить ассерт так, чтобы заодно кидал исключение "всё вааащщще плохо!" (и в релизе тоже).

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

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