cleanup to allow binaryen to be built in more strict environments#3566
Conversation
| return ASM_UNSIGNED; | ||
| } | ||
| } // fallthrough | ||
| [[fallthrough]]; |
There was a problem hiding this comment.
In which compilers does this work? A search suggests it's standardized in C++17, but maybe it was supported earlier?
And specifically, does it work in MSVC?
There was a problem hiding this comment.
I don't have the ability to build with MSVC at the moment, but it looks like it should be supported according to this page: https://docs.microsoft.com/en-us/cpp/cpp/attributes?view=msvc-160
There was a problem hiding this comment.
Thanks! It says VS2017+, but our docs on the readme page mention 2015. But maybe we can just update that to 2017?
Looks like emscripten CI uses 2019, so no issues there.
| } | ||
| default: { | ||
| } | ||
| default: { break; } |
There was a problem hiding this comment.
does the default really need a break if there is nothing after it?
There was a problem hiding this comment.
That one is not necessary, but the one above it is. I added it for clarity, but I'm happy to remove it if you'd prefer.
There was a problem hiding this comment.
Personally I think a break on the last item is more confusing actually. So I'd prefer to remove it.
|
What are the strict environments in question here? If we can't reproduce these errors on our CI, we will probably introduce new regressions in the future. |
|
I'm not sure if we have a place to mention a requirement for VS2017+. The readme mentions 2015 but I see it's in a link to other instructions. So maybe the docs are ok as they are. @tlively I think we are already building with |
|
There are some checks I can add to CMakeLists.txt. I'll add those and re-ping. |
|
I added two additional warnings, Those messages don't show up when the build passes. I'm not sure why that happens, or the best way to make these go away. If what I have is wrong please let me know. |
tlively
left a comment
There was a problem hiding this comment.
LGTM! I'm not too worried about extra errors that show up only in conjunction with correct errors, although that is strange.
|
Is this all set to be merged? Thanks! |
No description provided.