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

Warn about unused return values. #677

Merged
merged 2 commits into from Jun 27, 2016
Merged

Conversation

chriseth
Copy link
Contributor

No description provided.

char const* text = R"(
contract test {
function f() {
address(0x12).send(1);
Copy link
Member

Choose a reason for hiding this comment

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

actually really quick, just trying to understand this bit. Why does this one throw a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the return value of send is ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Part of me says this is smart, but then the other part of me says this can get annoying when it comes to void contracts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@VoR0220 Do void contracts ignore return values? (BTW, what's a void contract?)

Copy link
Member

Choose a reason for hiding this comment

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

Should have said void contract function. That is my terminology for a contract function that doesn't return a value. Although with the send...yeah you should be checking to make sure it happened.

Copy link
Contributor

Choose a reason for hiding this comment

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

address(0x12).send(1); this call's return value is ignored. this is the source of the warning, not f().

Copy link
Member

Choose a reason for hiding this comment

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

right, but that should be irrelevant if it's inside function f's scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if what I'm about to say is obvious or wrong. Bear with me as I'm learning lots at once. After a quick review of the code (disclaimer: I never learned C/C++ very well), it looks like this is part of the visitor pattern. If that's the case, then it looks like this code is being run on every single ExpressionStatement in the AST. So I think that means scope is irrelevant.

Copy link
Member

Choose a reason for hiding this comment

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

right, and I'm saying scope should be relevant.

Copy link
Member

Choose a reason for hiding this comment

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

actually...I just realized I'm wrong. This is good to merge.

@chriseth
Copy link
Contributor Author

Relaxed the warning to only be fired only for the low-level functions call, send, callcode and delegatecall.

@VoR0220
Copy link
Member

VoR0220 commented Jun 24, 2016

Much appreciated.

@VoR0220
Copy link
Member

VoR0220 commented Jun 24, 2016

much appreciated. Let me take another look.

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

3 participants