search

otrv4 / libotr-ng

A new implementation of OTR with support for version 4. This is a mirror of https://bugs.otr.im/otrv4/libotr-ng
Other
43 stars 9 forks source link

Remove redundant wrapper functions for otrng_bool #209

Closed osmk closed 5 years ago

osmk commented 5 years ago

The otrng_bool is compatible with the C standard boolean logic so the otrng_bool_is_true wrapper isn't needed. The conversion function c_bool_to_otrng_bool is unused across the code base and can also be safely removed.

The intent of this change is to declutter the code a bit. For otrng_bool_is_true to serve as an abstraction, one would have to apply it consistently wherever otrng_bool is examined. That would clutter the code for little practical value though so it seems better to just remove it.

It's possible of course that I misunderstand the purpose of otrng_bool, and perhaps there are future plans conflicting with this change. If so I apologise for the noise! :)

claucece commented 5 years ago

Hey!

Thanks for the PR. We are indeed planning on using everywhere otrng_bool_is_true. So no need for removing it.

Thanks!

claucece commented 5 years ago

I was actually thinking @osmk ... I was planning on doing the refacoring to always use otrng_bool_is_true... do you want to do it together?

osmk commented 5 years ago

@claucece I'd be happy to but I think I would have to understand the purpose of the abstraction first to do a good job of it :)

I get why there is a custom type as you need a consistent, platform independent way to encode a boolean for messages, but I don't fully understand why it can't be treated as simply compatible with Cs own boolean logic. I guess, if I had, I wouldn't have suggested removing it :)

I think 0=false and non-zero = true is part of the C standard and I don't know of any platform that inverts this, so it doesn't really give additional platform independence, does it?

I could see that you'd want to coerce booleans encoded in messages to exactly 1 or exactly 0, but that seems lite it would be easy to do in the serialisation code, rather than spread through the code?

Another potential issue is that just as there are no warnings today when you use an otrng_bool directly as a truth value, there won't be any such warnings even after a refactor, making it really easy for future changes to break the abstraction