-
Notifications
You must be signed in to change notification settings - Fork 128
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
fix HDF5Reader build with MSVC #1577
Conversation
@pnorbert, changed the base to release here, too. Same thing, though. |
Though, for this PR, it'd be good to hear back from @ax3l as to whether it really fixes the hdf5 engine build troubles he was seeing with MSVC, as I couldn't do that testing (and I think the adios2 CI doesn't, either.) I'm pretty sure that it fixes the one particular error he posted, and I found one more similar occasion, but there may be issues beyond that. |
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.
Don't forget the #include <vector>
additions for this change :)
Ok, there is a little bit more in the HDF5 files that we need to address: https://ci.appveyor.com/project/conda-forge/staged-recipes/builds/25636465
|
14f613b
to
94b0da2
Compare
Urgh, sorry I am of so little help. This fixed a lot again. Here we go with two more: https://ci.appveyor.com/project/conda-forge/staged-recipes/builds/25641834
|
Sadly, variable-sized stack arrays are only a feature of C99, but not C++11, so this patch uses std::vector instead.
including use of `uint` and more stack-allocated arrays
94b0da2
to
6bf7028
Compare
Rebased onto latest release, and fixed more variable length arrays -- should have gotten all this time via I should note that I'm doing these patches kinda in a rush, and sometimes not exactly sober, in between things like putting on a tuxedo and top hat to walk through 35 centigrade weather (maybe some Germans here understand...) Anyway, it certainly wouldn't be a bad idea to give these patches a good second look. |
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.
Please don't forget to #include <vector>
in all of these files :-)
I should have gotten them all now via `-Werror=vla` (with clang)
6bf7028
to
6fe4dbf
Compare
I'm pretty sure I did do all those includes this time ;) I just force-pushed to re-run he AppVeyor CI, which I think hit just a random hiccup. |
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.
Looks good to me and fixes the compile issues with MSVC! :)
Thanks a lot!
merging this PR (finally went through the CI) instead of #1587 |
fix HDF5Reader build with MSVC
Sadly, variable-sized stack arrays are only a feature of C99, but not C++11,
so this patch uses std::vector instead.
This fixes #1485, the second issue there, at least hopefully so -- @ax3l, could you test this one, too, as I don't think the adios2 CI covers this case. (I changed the place identified by the error message from your report, and one more place with the same issue, but there may be more).
@chuckatkins, I haven't checked, but my guess is that the MSVC CI doesn't build ADIOS2 with HDF5, otherwise I would have expected to see this problem earlier. So that may be something for the future todo list.