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

Fix -Wsign-compare warnings #1471

Merged
merged 2 commits into from
Aug 2, 2023
Merged

Fix -Wsign-compare warnings #1471

merged 2 commits into from
Aug 2, 2023

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Aug 1, 2023

No description provided.

@greg7mdp greg7mdp requested a review from heifner August 2, 2023 00:25
@greg7mdp greg7mdp requested a review from linh2931 August 2, 2023 13:01
@@ -36,6 +36,7 @@ if("eos-vm-oc" IN_LIST EOSIO_WASM_RUNTIMES)

set_source_files_properties(webassembly/runtimes/eos-vm-oc/LLVMJIT.cpp PROPERTIES COMPILE_FLAGS "--std=gnu++17")
set_source_files_properties(webassembly/runtimes/eos-vm-oc/LLVMEmitIR.cpp PROPERTIES COMPILE_FLAGS "--std=gnu++17")
set_source_files_properties(webassembly/runtimes/eos-vm-oc/switch_stack_linux.s PROPERTIES COMPILE_FLAGS "-Wno-unused-command-line-argument")
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a fine fix.. but I wonder why cmake is operating this way 🤔 it clearly knows that .s are assembly files -- not c/c++ files -- so why is it tacking on all the c/c++ options like -O3 -DNDEBUG etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

beats me!

@@ -530,7 +530,7 @@ BOOST_FIXTURE_TEST_CASE(test_session_fork, state_history_test_fixture) {
std::vector<eosio::state_history::block_position> have_positions;
eosio::state_history::state_result result;
// we should get 4 consecutive block result
for (int i = 0; i < 4; ++i) {
for (uint32_t i = 0; i < 4; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

size_t is likely better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, I'm not sure. The warning was when comparing with r.this_block->block_num which is a uint32_t.
If it was a comparison with <container>.size() then I would have used size_t.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought it was likely complaining about the written_data[i]. uint32_t then.

@@ -580,7 +580,7 @@ BOOST_FIXTURE_TEST_CASE(test_session_fork, state_history_test_fixture) {

eosio::state_history::state_result fork_result;
// we should now get data for fork 3,4
for (int i = 2; i < 4; ++i) {
for (uint32_t i = 2; i < 4; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

size_t is likely better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above.

Copy link
Member

@linh2931 linh2931 left a comment

Choose a reason for hiding this comment

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

Thanks for those large amount of edits.

@greg7mdp greg7mdp merged commit 36ffebc into main Aug 2, 2023
@greg7mdp greg7mdp deleted the compilation_warnings branch August 2, 2023 17:04
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.

4 participants