-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Conversation
d980fae
to
df92430
Compare
Thanks @selsta . I just left a minor comment about the possibility of adding |
df92430
to
562bdb8
Compare
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.
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 |
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.
Do you think it is a good idea to use unreachable
here (in addition to this dummy return)?
monero/src/crypto/variant4_random_math.h
Line 73 in ac02af9
#define UNREACHABLE_CODE __builtin_unreachable() |
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.
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 |
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.
Same comment as previous one.
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.
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.
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. |
Fixes compilation as we treat some warnings as errors as of #7481.
Closes #8622