[QGIS-Developer] clang-tidy warning for narrowing to float

[Julien Cabieces]

Hi,

I totally get your point, and I already faced such a situation where I
have to introduce (too) many static_cast<>. On the other side, I already
faced a situation where the use of this warning would have prevented a
real issue [0].

So, I'm not really in favor of your proposal because it would disable
the warning for the whole codebase, on some situation where it could be relevant. Could we not instead:
- introduce float getters/setters to have the conversion at only one place
- just have some converted variable "const float fvar =
static_cast<float>( var )". I don't think it makes the code so difficult
to read.

Regards,
Julien


[0] https://github.com/qgis/QGIS/pull/50016


> Hi all,
> I'd like to propose a change to QGIS's clang-tidy settings. Currently,
> we have bugprone-narrowing-conversions [0] turned on by wildcard,
> which by default warns on converting between integer types, between
> floating point types, and from integers to floating points, if the
> full range of the source type won't fit into the destination type
> without loss of precision.
>
> I fully agree that conversions between different integer types should
> be conscious decisions, since they can lead to security issues [1]. I
> find the floating point warnings to have dubious usefulness, though.
>
> In QGIS' 3D code, we often need to use integer values (e.g. screen
> size) in floating point calculations, with practically no chance that
> the integer value won't be representable as a float, and zero risk
> even if it is - creating a security issue would require a very
> creative abuse of floats. Fixing the warning leads to a visual spam of
> static_cast<float>()s everywhere, obscuring any actual bugs.
>
> Converting doubles to floats can actually cause issues, but it's also
> inevitable in many places, seeing as Qt3D uses floats, but we need to
> use doubles ourselves for precision reasons. Given this, I also think
> the warning isn't very useful here.
>
> I propose turning off WarnOnIntegerToFloatingPointNarrowingConversion
> and possibly WarnOnFloatingPointNarrowingConversion, based on your
> feedback.
>
> David Koňařík
>
> [1]:
> https://clang.llvm.org/extra/clang-tidy/checks/bugprone/narrowing-conversions.html
> [0]: e.g. char *copy_string(char *str, uint64_t str_size) {
>   uint32_t size_plus_null = str_size + 1;
>   char *copy = malloc(size_plus_null);
>   memcpy(copy, str, str_size);
>   copy[str_size] = '\0';
>   return copy;
> }
> _______________________________________________
> QGIS-Developer mailing list
> QGIS-Developer at lists.osgeo.org
> List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
> Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer

-- 

Julien Cabieces
Senior Developer at Oslandia
julien.cabieces at oslandia.com