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

Address some more defects. #588

Merged
merged 6 commits into from
Jan 3, 2015
Merged

Conversation

nomis52
Copy link
Member

@nomis52 nomis52 commented Jan 3, 2015

No description provided.

@@ -220,6 +220,7 @@ const RDMResponse *AckTimerResponder::ResponseFromQueuedMessage(
queued_response->ParamDataSize());
break;
case RDMCommand::SET_COMMAND_RESPONSE:
// coverity(SWAPPED_ARGUMENTS)
return new RDMSetResponse(
Copy link
Member

Choose a reason for hiding this comment

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

I was looking at this the other day. Shouldn't we create an RDMSetResponse constructor that takes a SetRequest and does half the leg work for you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that would be reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Well volunteered :)

Copy link
Member

Choose a reason for hiding this comment

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

Done! 👎

@peternewman
Copy link
Member

Fine aside from the comment.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling f4d7b8b on nomis52:master into 2ec510a on OpenLightingProject:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6101023 on nomis52:master into 2ec510a on OpenLightingProject:master.

nomis52 added a commit that referenced this pull request Jan 3, 2015
Address some more defects.
@nomis52 nomis52 merged commit 47721de into OpenLightingProject:master Jan 3, 2015
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.

3 participants