liballeg / allegro5

The official Allegro 5 git repository. Pull requests welcome!
https://liballeg.org
Other
1.9k stars 285 forks source link

ex_prim_wrap.c compile errors on MSVC #1553

Closed ReiquelApplegate closed 5 months ago

ReiquelApplegate commented 7 months ago

MSVC Version 17.9.5

52>E:\allegro5\examples\ex_prim_wrap.c(50,32): error C2057: expected constant expression
52>E:\allegro5\examples\ex_prim_wrap.c(50,35): error C2466: cannot allocate an array of constant size 0
52>E:\allegro5\examples\ex_prim_wrap.c(50,25): error C2133: 'vtxs': unknown size
52>E:\allegro5\examples\ex_prim_wrap.c(70,36): error C2057: expected constant expression
52>E:\allegro5\examples\ex_prim_wrap.c(70,39): error C2466: cannot allocate an array of constant size 0
52>E:\allegro5\examples\ex_prim_wrap.c(70,25): error C2133: 'vtxs': unknown size

constants or #defines seem like a better choice than creating a variable that is never going to change after it's initialized.

pedro-w commented 7 months ago

Does this mean no one has ever compiled ex_prim_wrap with MSVC? AFAICS GCC and clang are ok with this code, but MSVC won't work even when replacing int with const int. I guess it would have to be a #define - no constexpr for C, is there?

ReiquelApplegate commented 7 months ago

Who knows 🤷 MSVC doesn't support VLAs yet according to

https://zakuarbor.github.io/blog/variable-len-arr

So yeah I agree #defines will avoid relying on constant size VLAs which does sound like a very silly idea ;) And yes constexpr is a C++ thing

pedro-w commented 7 months ago

From godbolt I can see that const int compiles in MSVC in C++ mode, though that's irrelevant here. I'm surprised that no Windows users have come across this before now. Also I thought we had GitHub actions set up to test the build.

pedro-w commented 7 months ago

Found another couple of issue-ettes here: https://github.com/liballeg/allegro5/blob/85aa2f9a6274f645e4bfd7e78f70d3a4d20fc20f/examples/ex_prim_wrap.c#L172-L182 First, pixel_file may not be initialised (compiler complained about this) Second, the files ex_bitmap_wrap_pixel.* don't exist, I guess should be ex_prim_wrap_pixel.*. This gives me an error if I run ex_prim_wrap with the --shader command line option.

SiegeLord commented 7 months ago

The CI uses MSYS2. And for the pre-built binaries don't build examples (and Edgar's binaries are MinGW only), so that's how this happened.

pedro-w commented 7 months ago
commit 242a78f3dd3e4c20414e89dffe57df4347dc8ee5
Author: Peter Hull <peterhull90@gmail.com>
Date:   Tue Apr 23 20:19:45 2024 +0100

    Fixed compilation on MSVC - it doesn't support VLAs
    (even if the size is actually constant)
    Also specified the correct shader files and fixed unitialised
    variable warning

diff --git a/examples/ex_prim_wrap.c b/examples/ex_prim_wrap.c
index ae730c04e..75b4435e3 100644
--- a/examples/ex_prim_wrap.c
+++ b/examples/ex_prim_wrap.c
@@ -28,6 +28,8 @@ typedef enum {
    NUM_TYPES,
 } PRIM_TYPE;

+#define COUNT 32
+
 static void draw_square(ALLEGRO_BITMAP* texture, float x, float y, float w, float h, int prim_type)
 {
    ALLEGRO_COLOR c = al_map_rgb_f(1, 1, 1);
@@ -45,13 +47,12 @@ static void draw_square(ALLEGRO_BITMAP* texture, float x, float y, float w, floa
          break;
       }
       case POINTS: {
-         int n = 32;
-         float d = n - 1;
-         ALLEGRO_VERTEX vtxs[n * n];
+         float d = COUNT - 1;
+         ALLEGRO_VERTEX vtxs[COUNT * COUNT];

-         for (int iy = 0; iy < n; iy++) {
-            for (int ix = 0; ix < n; ix++) {
-               ALLEGRO_VERTEX *v = &vtxs[iy * n + ix];
+         for (int iy = 0; iy < COUNT; iy++) {
+            for (int ix = 0; ix < COUNT; ix++) {
+               ALLEGRO_VERTEX *v = &vtxs[iy * COUNT + ix];
                v->x = x + 5. * w * (float)ix / d;
                v->y = y + 5. * h * (float)iy / d;
                v->z = 0.;
@@ -61,15 +62,14 @@ static void draw_square(ALLEGRO_BITMAP* texture, float x, float y, float w, floa
             }
          }

-         al_draw_prim(vtxs, NULL, texture, 0, n * n, ALLEGRO_PRIM_POINT_LIST);
+         al_draw_prim(vtxs, NULL, texture, 0, COUNT * COUNT, ALLEGRO_PRIM_POINT_LIST);
          break;
       }
       case LINES: {
-         int n = 32;
-         float d = n - 1;
-         ALLEGRO_VERTEX vtxs[2 * n * 2];
+         float d = COUNT - 1;
+         ALLEGRO_VERTEX vtxs[2 * COUNT * 2];

-         for (int iy = 0; iy < n; iy++) {
+         for (int iy = 0; iy < COUNT; iy++) {
             for (int ix = 0; ix < 2; ix++) {
                ALLEGRO_VERTEX *v = &vtxs[ix + 2 * iy];
                v->x = x + 5. * w * (float)ix;
@@ -81,8 +81,8 @@ static void draw_square(ALLEGRO_BITMAP* texture, float x, float y, float w, floa
             }
          }
          for (int iy = 0; iy < 2; iy++) {
-            for (int ix = 0; ix < n; ix++) {
-               ALLEGRO_VERTEX *v = &vtxs[2 * ix + iy + 2 * n];
+            for (int ix = 0; ix < COUNT; ix++) {
+               ALLEGRO_VERTEX *v = &vtxs[2 * ix + iy + 2 * COUNT];
                v->x = x + 5. * w * (float)ix / d;
                v->y = y + 5. * h * (float)iy;
                v->z = 0.;
@@ -91,7 +91,7 @@ static void draw_square(ALLEGRO_BITMAP* texture, float x, float y, float w, floa
                v->color = c;
             }
          }
-         al_draw_prim(vtxs, NULL, texture, 0, 4 * n, ALLEGRO_PRIM_LINE_LIST);
+         al_draw_prim(vtxs, NULL, texture, 0, 4 * COUNT, ALLEGRO_PRIM_LINE_LIST);
          break;
       }
    }
@@ -169,15 +169,15 @@ int main(int argc, char **argv)
       if (!shader)
          abort_example("Error creating shader.\n");

-      const char* pixel_file;
+      const char* pixel_file = NULL;
       if (al_get_shader_platform(shader) == ALLEGRO_SHADER_GLSL) {
    #ifdef ALLEGRO_CFG_SHADER_GLSL
-         pixel_file = "data/ex_bitmap_wrap_pixel.glsl";
+         pixel_file = "data/ex_prim_wrap_pixel.glsl";
    #endif
       }
       else {
    #ifdef ALLEGRO_CFG_SHADER_HLSL
-         pixel_file = "data/ex_bitmap_wrap_pixel.hlsl";
+         pixel_file = "data/ex_prim_wrap_pixel.hlsl";
    #endif
       }
SiegeLord commented 5 months ago

This got fixed in https://github.com/liballeg/allegro5/commit/9ed326a43ea07b9ac7e44a4fc488e651e59af4b1