Remove designated array initializers in rz_analysis.h
#3876
+40
−14
Conversation
This file contains 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
DMaroo
requested review from
ret2libc,
thestr4ng3r,
wargio,
XVilka and
kazarmy
as code owners
DMaroo
force-pushed
the
split-rz-analysis-op
branch
2 times, most recently
from
ced25a5 to
4a9e550
Compare
3 tasks
wargio
reviewed
Sep 20, 2023
XVilka
requested changes
Sep 20, 2023
|
The culprit seems to be in this code: minimal example on Compiler Explorer |
|
Discussion continued on Mattermost: https://im.rizin.re/rizinorg/pl/4gn8eymqupbtjnr9rahd1stzwc. |
|
Please undo the split and fix the MSVC build, preferably with adding MSVC C++ CI job too, then we can merge. |
DMaroo
force-pushed
the
split-rz-analysis-op
branch
from
125fd68 to
af27b59
Compare
DMaroo
changed the title
RzAnalysisOpType in a different file
DMaroo
changed the title
XVilka
requested changes
Sep 22, 2023
There was a problem hiding this comment.
clang-format complains:
Formatting [1277/1629] librz/include/rz_analysis.h
librz/include/rz_analysis.h:717:26: error: code should be clang-formatted [-Wclang-format-violations]
RZ_ANALYSIS_VAR_KIND_END ///< Number of RzAnalysisVarKind enums
^
librz/include/rz_analysis.h:734:28: error: code should be clang-formatted [-Wclang-format-violations]
RZ_ANALYSIS_VAR_ORIGIN_END ///< Number of RzAnalysisVarOriginKind enums
^
XVilka
requested changes
Sep 22, 2023
DMaroo
changed the title
rz_analysis.h
|
Will remove the draft status after the PR for adding the CI gets merged. |
What PR? |
|
I'm working on it 😅 |
To be honest, I think it's better to add the CI job in the same PR since it will be red on the |
|
Alright, makes sense! |
DMaroo
force-pushed
the
split-rz-analysis-op
branch
from
819ee5c to
4dd63d4
Compare
* Need to test whether Rizin headers can be included in C++ when
using MSVC, needed for Cutter to build successfully
* Changed the name of `build-cpp` CI job in GitHUb jobs to
`build-cpp-linux` to make it obvious that it only performs the
test for Linux
DMaroo
force-pushed
the
split-rz-analysis-op
branch
from
4dd63d4 to
6ae791b
Compare
|
I have only enabled the C++ build test for VS2022, should I also enable it for |
XVilka
approved these changes
Sep 25, 2023
ret2libc
pushed a commit
that referenced
this pull request
Sep 27, 2023
* Do not use designater array initializer because MSVC complains
* Add new Appveyor CI test for including C headers from C++ files
* Need to test whether Rizin headers can be included in C++ when
using MSVC, needed for Cutter to build successfully
* Changed the name of `build-cpp` CI job in GitHUb jobs to
`build-cpp-linux` to make it obvious that it only performs the
test for Linux
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.
Your checklist for this pull request
Detailed description
Removed designated array initializers from the code. MSVC C++ compiler seems to not be able to compile it since it is a C99 thing, and MSVC C++ does not entirely support C99.
MCVE: https://godbolt.org/z/af3v7rWfh.
Also added a new test in Appveyor for testing inclusion of Rizin headers in C++ files. The test is only enabled for VS2022.
Test plan
The new CI for compiling the Rizin headers included in a C++ passes succesfully, and green CI.
Closing issues
Fixes the build for rizinorg/cutter#3247.
(Ignore the branch name)