c42f / tinyformat

Minimal, type safe printf replacement library for C++
531 stars 75 forks source link

Fix: type of FormatListN template parameter #74

Closed jrohel closed 4 years ago

jrohel commented 4 years ago

The fix changes type of FormatListN template parameter from int to std::size_t.

I assume that FormatListN template parameter is allways non-negative integer number. And sizeof... operator returns a constant of type std::size_t.

Without the fix and with strict gcc compiler settings we get:

tinyformat.h:1019:21: error: conversion from ‘long unsigned int’ to ‘int’ may change value [-Werror=conversion]
 1019 | detail::FormatListN<sizeof...(Args)> makeFormatList(const Args&... args)
      |                     ^~~~~~~~~~~~~~~
tinyformat.h: In function ‘tinyformat::detail::FormatListN<sizeof... (Args)> tinyformat::makeFormatList(const Args& ...)’:
tinyformat.h:1021:32: error: conversion from ‘long unsigned int’ to ‘int’ may change value [-Werror=conversion]
 1021 |     return detail::FormatListN<sizeof...(args)>(args...);

Fix solves the problem reported by gcc. But solution is not 100% correct. The N is passed to the FormatList constructor that takes and stores int. So, may be we get next message from compiler in the future (eg. from newer compiler). Correct is to store N to std::size_t. Or if we want to save some memory, use int and add static_cast<int>.

When I think more about it. Maybe it's a compiler error. It knows the value of N at compile time, so it shouldn't report errors / warnings that something could happen. It can't happened in my compiled code. Huh, I'm confused now.

c42f commented 4 years ago

It's a spurious warning for sure, but the fix is harmless enough thanks :+1:

Presumably this size_t is never actually stored because it's part of the type. Even if it were, it would be on the stack. So sizeof(size_t) shouldn't be a problem here.