Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses compilation issues with MSVC C++20 by adding header guards, function inlining, and resolving compiler-specific problems with array sizing and template processing.
- Added header guards and inline specifiers to prevent multiple definition errors
- Fixed zero-sized array compilation error by ensuring minimum array size of 1
- Replaced problematic std::ceil/std::log2 usage with constexpr-compatible alternatives
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| include/wamr.hpp | Added header guards and inline specifiers to all functions |
| include/cmcpp/traits.hpp | Added constexpr helper functions and fixed template recursion for variant processing |
| include/cmcpp/lower.hpp | Changed uint parameter type to uint32_t for consistency |
| include/cmcpp/lift.hpp | Changed uint parameter type to uint32_t for consistency |
| include/cmcpp/context.hpp | Fixed func_t template syntax from comma-separated to function signature format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void trap(const char *msg) | ||
| inline void trap(const char *msg) | ||
| { | ||
| throw new std::runtime_error(msg); |
There was a problem hiding this comment.
Throwing a pointer to an exception object instead of the exception object itself. This should be throw std::runtime_error(msg); without the new keyword.
Suggested change
| throw new std::runtime_error(msg); | |
| throw std::runtime_error(msg); |
|
|
||
| constexpr size_t output_size = std::is_same<result_t, void>::value ? 0 : 1; | ||
| wasm_val_t outputs[output_size]; | ||
| wasm_val_t outputs[output_size == 0 ? 1 : output_size]; |
There was a problem hiding this comment.
[nitpick] The ternary operator used to avoid zero-sized arrays is unclear. Consider using a named constant or adding a comment explaining why this workaround is necessary for MSVC C++20 compatibility.
Suggested change
| wasm_val_t outputs[output_size == 0 ? 1 : output_size]; | |
| // MSVC does not allow zero-sized arrays, so we use 1 when output_size is zero. | |
| constexpr size_t OUTPUT_ARRAY_SIZE = (output_size == 0 ? 1 : output_size); | |
| wasm_val_t outputs[OUTPUT_ARRAY_SIZE]; |
Comment on lines
+713
to
+715
| process_types<StartIndex + 1, Rest...>(flat); | ||
| } | ||
| } |
There was a problem hiding this comment.
The recursive call to process_types is inside the for loop, which will cause multiple recursive calls for each element in ValTrait<First>::flat_types. The recursion should happen after processing all elements of the current type.
Suggested change
| process_types<StartIndex + 1, Rest...>(flat); | |
| } | |
| } | |
| } | |
| } | |
| process_types<StartIndex + 1, Rest...>(flat); |
Owner
|
Thanks for the submission! The windows CI build failure looks unrelated to this PR and I would like to see that working before I merge (even if I just see it passing on my machine), I will try and look in the next could of days... |
Owner
|
Can you check the failing windows build (I fixed the vcpkg configuration failing)? |
Contributor
Author
I have some error: |
Contributor
Author
|
vcpkg configure is so slow |
Owner
|
It is slow, but it does do a good job of caching the builds. Can you share the content of F:\code2\dodo\component-model-cpp\build\vcpkg-manifest-install.log? |
Contributor
Author
|
log is here |
Owner
|
Can you do a: You may need to do |
This reverts commit eae9cad.
Contributor
Author
|
strange. vcpkg download wasi-sdk-${VERSION}-x86_64-windows.tar.gz on my compute the hash is ee91880d1be2a9d2f3bd90c1893448c8861130a0726811de0f55f90394260aa4693f7efbee81de5bd599c4395fe26809642741033e8e062894c9c4dea249bae4. but on ci it is e8bdae827dbbb967bf9815603aeff76ac40344c79cf6a1c388e63931c77cdc5560860c6f2ec74f3c7895fab08b93940f60e9e26365b6f4ba354ca3a921803be7. |
Owner
|
You might want to rebase your local onto the latests trunk? |
Owner
|
Closing as trunk now builds and passes unit tests on windows. |
GordonSmith
added a commit
that referenced
this pull request
Oct 14, 2025
…dex> template - Added empty_case<Index> template in variant.hpp for unique empty variant cases - Modified code_generator.cpp to use empty_case<Index> instead of bare monostate - Added proper ValTrait specialization for empty_case with ValType::Void - Result: 180/199 stubs compile (down from 182 baseline) - Main issue (duplicate monostate) resolved, but variants stub shows new error (assignment operator) - Need to investigate variant assignment operator issue next
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.