-
Notifications
You must be signed in to change notification settings - Fork 45
Replace VLA for MSVC compatibility #177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
projectitis
commented
Feb 28, 2022
- Replaced VLA with alternative
- Now able to compile successfully using Visual Studio
|
@mikerreed @chris-rive we were discussing this a while ago @chris-rive maybe you can take a look at whether there's a configuration flag or something that can enable variable-length arrays in MSBuild? This has bitten us before and it kind of sucks to have to resort to allocating them on the heap every time (Mike mentioned Skia has a cool abstraction that allocated on the stack and grows to the heap only if necessary that maybe we should adopt). |
|
MSVC has _alloca and _malloca for stack allocation. I'm guessing that's what the abstraction layer would resort to. |
If you install Visual Studio with Clang tools, you can use LLVM as your platform toolset instead of MSVC. I wonder if this would allow us to use variable length arrays and other Clang features? Using GN on Skia we were able to build entirely with Ninja/Clang, and just set up a custom build command for working inside Visual Studio. I would love it if we could do that here as well. Clang has much better support for SIMD, whether that's SSE, NEON, or WASM, so I would really push for us to drop MSVC support and instead focus on figuring out how to use Clang everywhere.
I think we could accomplish the same thing using a custom allocator with std::vector, which would be more "standard" and future proof, I believe. I like the concept and think we will probably want it, even if we also get in support for variable length arrays. |
| // TODO: replace these with stack-alloc helpers? | ||
| ColorInt colors[count]; | ||
| float stops[count]; | ||
| ColorInt* colors = new ColorInt[count]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a limit on how long "count" can be? If not we probably do want a wrapper that uses stack storage up to a certain threshold, then falls back on heap storage.
I can confirm that the codebase compiles out-of-the-box with clang-cl using visual studio. The command line options work, and VLAs are fine. There are about a million warnings, but I get a rive.lib at the end of it. I ran The warnings are a symptom of the So if you are thinking of ditching MSVC support, I'm happy to close this PR? Otherwise I can suggest an alternative that uses |
|
@luigi-rosso Is the 'official' ( 😁 ) support on windows for clang-cl and not MSVC? |