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

[SH] Fix build warnings #1960

Merged
merged 7 commits into from Apr 6, 2023
Merged

Conversation

Rot127
Copy link
Collaborator

@Rot127 Rot127 commented Feb 24, 2023

These commits add several build warning options to CMakeList.txt and fixes build warnings for the SH arch.
Two unused functions are removed and a parantheses/int-in-bool-contexxt warning is fixed.

@ysat0 Could you please take a look at it as well?

@Rot127
Copy link
Collaborator Author

Rot127 commented Feb 24, 2023

Anyone has an idea why MSVC != true for the failing test? Unfortunately, I currently don't have a Windows machine for debugging.

@ysat0
Copy link
Contributor

ysat0 commented Feb 25, 2023

I also don't have an environment where I can use MSVC right now.
I will prepare and check it in the next week.

@ysat0
Copy link
Contributor

ysat0 commented Mar 2, 2023

Fixed.
ysat0@0eebf32
It looks like cmake can't detect MSVC.
I've verified that this change builds on Windows/MSVC and Linux/gcc.

@Rot127
Copy link
Collaborator Author

Rot127 commented Mar 6, 2023

The build seems to fail with this as well. Or am I missing something?
We could also just remove the build options and open an issue about it. If everyone is fine with it.

CMakeLists.txt Outdated
@@ -1,6 +1,12 @@
# For MSVC_RUNTIME_LIBRARY
cmake_minimum_required(VERSION 3.15)

if (MSVC)
Copy link
Member

@kabeor kabeor Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, you need to define what MSVC is here

Copy link
Collaborator Author

@Rot127 Rot127 Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be predefined by cmake. See: https://cmake.org/cmake/help/latest/variable/MSVC.html
But it isn't in our case.

Copy link
Collaborator Author

@Rot127 Rot127 Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait. I haven't tried this last time. Doesn't work

@XVilka
Copy link
Contributor

XVilka commented Apr 5, 2023

@Rot127 I think I might know the reason - this variable probably doesn't work before the project() clause.

See also https://stackoverflow.com/questions/41692725/cmake-doesnt-recognize-msvc-compiler#41693234

@Rot127
Copy link
Collaborator Author

Rot127 commented Apr 6, 2023

This was it. Thanks!

Copy link
Contributor

@XVilka XVilka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kabeor
Copy link
Member

kabeor commented Apr 6, 2023

Great, Thank you!

@kabeor kabeor merged commit 80dae3f into capstone-engine:next Apr 6, 2023
5 of 7 checks passed
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.

None yet

4 participants