Post by Richard AshOn Mon, 19 Sep 2016 10:50:04 -0400
Post by Roger DannenbergES.100: Don't mix signed and unsigned arithmetic
That seems pretty clear to me. Compilers can give warnings, but if
humans can't understand the code that's warned about, how does that
help?
Remember that size_t is an unsigned type. So the corollary of
Stroustrup's guidance is that all arithmetic which uses size_t should
be unsigned.
Indeed, one might, pedantically, add 1u not 1, say, wherever you combine it
with a constant.
Post by Richard AshPost by Roger DannenbergFrom my work background (semi-embedded industrial) I would question any
code which does an unchecked conversion between a wider (at either end)
type and a narrower type.
And yes, my sweep of uses of 64 bit sampleCount used as the size of an
in-memory buffer, was all about suspicious narrowing conversions.
Post by Richard AshAny conversion involving signed and
unsigned values of the same width loses at one end or the other, so
there needs to be a check at the conversion. Locating these is a very
good use of the compiler warning, and once the check exists, an
explicit cast gets rid of the warning, and makes the conversion obvious
to anyone reading the code.
The clang compiler with XCode can provide this, if you change some settings
for the Audacity project file. I am busy trying to sweep the warnings
right now. Many of which come from lib-src though.
But -- habitual casting only papers over the problem and should be
discouraged. I think that wherever casting happens, there should be at
least a comment explaining why the conversion is justified, or more
strongly an assertion. You should be able to prove from the context that
the narrowed value will not be out of bounds.
Post by Richard AshSo as far understanding goes, any use of intentional overflow / modulo
arithmetic needs comments to explain it anyway, and certainly should
use consistent type. In C++ code for multiple platforms I would regard
that sort of trick as inappropriate anyway - it belongs to
single-target microcontroller code in base C.
Post by Roger DannenbergI think you have to look at every case separately, but if you
need unsigned because 31 bits is not enough, but 32 /is/ enough,
maybe the code would be more maintainable just using a 64-bit signed
integer.
If range is the only consideration, and you are free to choose the
types (no libraries involved) then I would agree. However once you
start using libaries which have APIs defined in terms of size_t, that
flexibility is lost (although size_t may well be an unsigned 64-bit int
in some cases ...)
Libraries? It's not just about third party libraries. size_t is baked
into the language itself, as the type passed to operator new, and also
baked into the standard C++ library.
Post by Richard AshOn Mon, 19 Sep 2016 11:17:30 -0400
Post by Roger DannenbergsampleCount now gets converted to size_t only where difference of two
sample counts are taken, and there is some informal proof (commented)
that the result is small and not negative.
With my work hat on, I'd be giving you a hard time about this. I would
normally want either an assert or a runtime error handler on this sort
of conversion. I haven't reviewed the actual commits, but I have spent a
lot of time with bugs caused by things that 'won't happen'.
See what I have done to sampleCount. It is now a class, not just an alias
for 64 bit integer. It does not convert implicitly to numerical types, but
compels you to call a member function called as_double, as_float,
as_long_long, or as_size_t. And the last does assert dynamically that the
conversion is within bounds, at least in debug builds.
PRL
Post by Richard AshRichard
------------------------------------------------------------
------------------
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel