Make it a little easier for Linux users #136
Conversation
|
The C++ issues are not so much Windows-specific as MSVC-specific. I think however that the MSVC emu2413/sample.cpp is not actually needed, it would be better to exclude it from CMake or even delete it... As all the source is compiled as C++, isn't it more correct to The CMake parts are mostly fine, it might be better to peg the Allegro version to what we use in Windows though? The way it gets zlib, freetype etc is more Unixy and thus the CMake build in Windows may not work so well, but then I guess that's not the goal here. |
| case 'd': | ||
| // Signed decimal | ||
| formatted_length = sprintf_s(buffer, buffer_size, "%d", is_byte ? (s8)data : (s16)data); | ||
| formatted_length = snprintf(buffer, buffer_size, "%d", is_byte ? (s8)data : (s16)data); |
There was a problem hiding this comment.
Maybe we should be thinking more C++20 and use std::format here? I know Omar is somewhat allergic to std::string though...
There was a problem hiding this comment.
The code looked pretty C, so I wanted to stick with it. Converting to C++ is not the main thing of this MR.
I'd guess such an MR would be significantly bigger 😉
I also did not care too much about the return value on failures. buffer is always != NULL , buffer_size is more then sufficient and the format is also always valid. The remaining code also seemed not to care about negative return values...
There was a problem hiding this comment.
We can simply wrap snprintf. And I believe it is more convenient to use than std::format yes.
I sometimes use my own helper https://github.com/ocornut/str May drag this into the codebase eventually.
As all the source is compiled as C++, isn't it more correct to #include
It provides no added value, and traditionally any C++ headers even the simplest wrapper stuff makes compilation a little slower.
|
Regarding
Yes, it is, but the whole code is pretty C'ish and that would conflict (style-wise) with all the other includes. Regarding
I will look into it |
…all under win and mac
|
I added more "these packages should be installed for allegro", how the pacakages can be installed in win and mac. I'd need a certain label or commit hash to what you use usually to include it in the cmake file... |
3 participants
Here I fix 2 things:
1: There is now a CMake File that kindof builds (at least on my system). It also fetches allegro.
2: It changes some windows-specific calls to C++ standard calls that also work on linux