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

storages: add missing return #9103

Merged
merged 1 commit into from Jan 18, 2024
Merged

Conversation

selsta
Copy link
Collaborator

@selsta selsta commented Dec 25, 2023

Fixes compilation as we treat some warnings as errors as of #7481.

Closes #8622

@0xFFFC0000
Copy link
Collaborator

Thanks @selsta . I just left a minor comment about the possibility of adding unreachable.

Copy link
Collaborator

@0xFFFC0000 0xFFFC0000 left a comment

Choose a reason for hiding this comment

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

Thanks selsta. Passed the tests.

@@ -231,6 +231,7 @@ namespace epee
default:
CHECK_AND_ASSERT_THROW_MES(false, "unknown entry_type code = " << type);
}
return read_ae<int8_t>(); // dummy return to avoid compiler warning
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it is a good idea to use unreachable here (in addition to this dummy return)?

#define UNREACHABLE_CODE __builtin_unreachable()

Copy link
Collaborator

Choose a reason for hiding this comment

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

After a brief discussion with selsta in IRC, we decided to not code more complicated. We might add it later.

@@ -322,6 +323,7 @@ namespace epee
default:
CHECK_AND_ASSERT_THROW_MES(false, "unknown entry_type code = " << ent_type);
}
return read_se<int8_t>(); // dummy return to avoid compiler warning
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as previous one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as a comment on previous issue.

After a brief discussion with selsta in IRC, we decided to not code more complicated. We might add it later.

@iamamyth
Copy link

Agreed that unreachable would be good to add (as a broadly available macro, because such cases are common), otherwise the new default behavior could hide logic errors. As it really should be available project-wide, I don't object to making the change in a later PR.

@luigi1111 luigi1111 merged commit 663dcf3 into monero-project:master Jan 18, 2024
18 checks passed
@selsta selsta deleted the missing-return branch February 20, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'make debug' fails
4 participants