Hi Roger,
The reason these stand out is that there are an awful lot of static analysis
warnings (over 400) relating to unnecessary (and potentially buggy) sign
conversions, and most of these are due to the use of signed integers where
unsigned would typically be used.
With which tool? The library component itself is clean with cppcheck,
Coverity Scan, CLang static analyzer and CodeQL as we use them in GDAL
with a vendored copy of libtiff.
And for the standard memory allocation functions, these all use size_t, not a
signed size type like ssize_t.
The same consideration may also apply to other functions where we are using the
type to represent sizes which can not be negative, where unsigned would be more
natural to use and in many cases all of the calculations are done with unsigned
inputs.
I was wondering if anyone could provide some historical context for why the API
is this way. And as a followup, if it would be something we could consider
changing to make the API and the internal implementation safer and cleaner.
I've no historical context on that either. I'm wondering how practical a
change is at that point of history given that it impacts API as well. I
suspect that there might part of the code base where the signedness
would be relied on, typically for loops (e.g. "tmsize_t i = stride; for
(; i < cc - 3; i += 4)" at line 371 of tif_predict.c that would not run
correctly on unsigned type if cc < 3), and switching to unsigned could
introduce bugs. Not a strong opinion though, just a remark that the
benefit vs risk at that point of time isn't immediately obvious to me.
Over the next few weeks, I’m going to be looking at opening a series of merge
requests to address various categories of static analysis warning in the
codebase, plus enabling specific compiler warnings by default to check for them
by default in the CI builds. The aim being to improve the quality and
robustness of the codebase. Most of these are fairly trivial to implement, but
questions such as the above really depend upon what the original intentions
were and any compatibility constraints we have to factor in.
+1
I’m also going to take a look through all of the currently open merge requests.
There are quite a few which are several years old, and I will re-review and
close where it’s clear we will not be merging them in their current form.
Thanks for that
Even
--
http://www.spatialys.com
My software is free, but my time generally not.
_______________________________________________
Tiff mailing list
[email protected]
https://lists.osgeo.org/mailman/listinfo/tiff
