Skip to content

Fix for variable length array initialization error.#986

Closed
YLouWashU wants to merge 1 commit intostevenlovegrove:masterfrom
YLouWashU:fix/vla-init-error
Closed

Fix for variable length array initialization error.#986
YLouWashU wants to merge 1 commit intostevenlovegrove:masterfrom
YLouWashU:fix/vla-init-error

Conversation

@YLouWashU
Copy link

This PR addresses the following issue:

On Mac-ARM64 computer when building from source, I got the following errors:

/tmp/Pangolin/components/pango_display/src/view.cpp:315:16: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
  315 |     GLfloat zs[zsize];
      |                ^~~~~
/tmp/Pangolin/components/pango_display/src/view.cpp:315:16: note: initializer of 'zsize' is not a constant expression
/tmp/Pangolin/components/pango_display/src/view.cpp:314:15: note: declared here
  314 |     const int zsize = zl*zl;
      |               ^
1 error generated.

This is caused by compiler not allowing creation of variable length array.

This PR addresses this by creating a pointer that points to the data member of a std::vector.

@christian-rauch
Copy link
Collaborator

Would this also work if you use constexpr? And could you update the macOS runner first to reproduce the issue you are trying to fix?

@YLouWashU
Copy link
Author

Would this also work if you use constexpr? And could you update the macOS runner first to reproduce the issue you are trying to fix?

I can't use constexpr because 'zl' is not compile time const. And also, can you remind me what is mac runner? Thanks!

@christian-rauch
Copy link
Collaborator

christian-rauch commented Jun 11, 2025

I assume you are trying to fix #985. There, we hypothesised that this is likely caused by newer macOS / AppleClang version. The CI is using macOS 14 and the issue seems to appear on macOS 15. So updating the CI pipeline here:

os: [ubuntu-22.04, ubuntu-24.04, macos-14, windows-2022]
should reproduce the issue and your PR should fix the issue.

Comment on lines +315 to +316
std::vector<GLfloat> zsVec(zsize);
GLfloat* zs = zsVec.data();
Copy link
Collaborator

@christian-rauch christian-rauch Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you allocate the memory through a std::vector and not malloc/free?

Or the other way around, if you already allocated the memory for the std::vector<GLfloat> why aren't you reusing this vector and just use the data pointer wherever zs is used?

Also, when you turn this into a dynamic array, you can remove the MSVC workaround above.

@christian-rauch
Copy link
Collaborator

I updated the runner, enabled VLA errors to have this triggered when new VLAs are added, and I replaced the zs VLA as suggested with a std::vector<> in #987. Can you check that PR if it solves your issue?

@YLouWashU
Copy link
Author

I updated the runner, enabled VLA errors to have this triggered when new VLAs are added, and I replaced the zs VLA as suggested with a std::vector<> in #987. Can you check that PR if it solves your issue?

Thanks so much for the fix Christian! I have tested your PR on my end and it is working. Let's land yours instead. I will close this PR.

Closing because #987 is a more proper fix.

@YLouWashU YLouWashU closed this Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants