jrl-umi3218 / jrl-mal

Matrix Abstract Layer to allow multiple libraries on JRL code
GNU Lesser General Public License v3.0
5 stars 5 forks source link

printf of a size_t element #3

Closed francois-keith closed 13 years ago

francois-keith commented 13 years ago

Hello everyone

In the file boostmatrix.hh

There is a fprintf that causes a lot of warnings, line 896.

        fprintf(stderr,"!!\tmaal::Matrix: error in matrix size for product [%dx%d] x [%dx%d].\n",
            mat1.size1(),mat1.size2(),mat2.size1(),mat2.size2()); fflush(stderr);

This raises a LOT of warnings in MacOSX systems since size_t elements should be printed with the option %zu or %lu, not %d. Yet, the option %zu (or %z) is not windows-compatible. Remains the option %lu, but apparently (according to http://stackoverflow.com/questions/1546789/clean-code-to-printf-size-t-in-c-or-nearest-equivalent-of-c99s-z-in-c), it can raise an issue with 64-bits systems (I tried on windows and macOSx without any problems though).

It really like to get rid of those warnings, but I don't know whether keeping the %lu option or using the brute-force solution consisting in casting the size_t into (int)

Any ideas?

thomas-moulard commented 13 years ago

Hello François, the easiest way is to switch these printf to Boost.Format (http://www.boost.org/doc/libs/1_41_0/libs/format/index.html). This library is compatible with POSIX printf, portable, we already rely on Boost and solve many drawbacks of printf (strong typing, etc.). I strongly encourage everyone to switch as many printf statements to Boost.Format as possible.

francois-keith commented 13 years ago

Hello Thomas,

Maybe a silly question, but why do we not use std::cerr / std::cout rather than fprintf ?

thomas-moulard commented 13 years ago

Short answer: if what you display is simple, yes this is the best solution.

Long answer: it is bad to use printf for several reasons: bad formatting string can lead to memory errors, mixing C/C++ I/O can result in difficult to debug problems such as buffering issues. etc. However printf is way easier to read when dealing with complex display. The formatting string is a powerful tools and does not rely on a global state such as iostream modifiers.

Considering that the strings are using the printf format and that boost.format is compatible with the POSIX printf standard, it would not harm to use boost.format here and would be IMHO easier to read:

std::cout << "[" << size1 << "x" << size2 << "]"
// VS
boost::format ("[%dx%d]") % size1 % size2;

But this is more a general remark than a requirement for this simple case :)

francois-keith commented 13 years ago

Corrected by commit 63b8084b2aad23cd488c