Skip to content
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

Replace VLA for MSVC compatibility #177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

projectitis
Copy link

  • Replaced VLA with alternative
  • Now able to compile successfully using Visual Studio

@luigi-rosso
Copy link
Collaborator

@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).

@projectitis
Copy link
Author

MSVC has _alloca and _malloca for stack allocation. I'm guessing that's what the abstraction layer would resort to.

@csmartdalton
Copy link
Contributor

whether there's a configuration flag or something that can enable variable-length arrays in MSBuild?

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.

Skia has a cool abstraction that allocated on the stack and grows to the heap only if necessary that maybe we should adopt

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.

@@ -82,14 +82,17 @@ void LinearGradient::update(ComponentDirt value) {
const double ro = opacity() * renderOpacity();
const auto count = m_Stops.size();
// TODO: replace these with stack-alloc helpers?
ColorInt colors[count];
float stops[count];
ColorInt* colors = new ColorInt[count];
Copy link
Contributor

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.

@projectitis
Copy link
Author

projectitis commented Mar 1, 2022

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 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.
EDIT: I tell a lie. The command line options -fno-exceptions and -fno-rtti are still ignored by clang-cl (-Wunknown-argument). But I still get a lib.

I ran premake5 vs2019 and then loaded the solution file in visual studio and made a few changes to the options, including selecting clang-cl as the toolset.

The warnings are a symptom of the -Wall flag. I've found out it is recommended to use -W4 instead with clang otherwise every other line gives you a "Not compatible with c++98" warning :(

So if you are thinking of ditching MSVC support, I'm happy to close this PR? Otherwise I can suggest an alternative that uses _malloca (docs). This function attempts to allocate via stack, and if not possible, uses heap instead.

@projectitis
Copy link
Author

@luigi-rosso Is the 'official' ( 😁 ) support on windows for clang-cl and not MSVC?
If so, I will close this PR.

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.

3 participants