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

Invert logic of RTF_*_IF macros #84

Merged
merged 5 commits into from
Oct 6, 2017
Merged

Conversation

damn1
Copy link
Contributor

@damn1 damn1 commented Oct 5, 2017

Change logic for RTF_dosomething_IF(condition, message) macros, adding new macros RTF_dosomething_IF_TRUE and RTF_dosomething_IF_FALSE, deprecating the old ones with a warning message.

@traversaro traversaro changed the base branch from master to devel October 5, 2017 16:20
@traversaro
Copy link
Member

@damn1 I changed the PR to target devel, I imagine was you original intention.

* test failed.
*/
#define RTF_ASSERT_FAIL_IF(condition, message) \
RTF_TEST_REPORT("WARNING: RTF_ASSERT_FAIL_IF(condition, message) "\
Copy link
Member

Choose a reason for hiding this comment

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

It would be ideal if we could turn this in a compilation warning rather than a runtime warning. Let me know if it is not possible, in that case I will approve the PR as it is. : )

@traversaro
Copy link
Member

@damn1 check the output of AppVeyor, it seems that the errors are failing for some reason.

@@ -11,6 +11,9 @@ Important Changes
* Fixed old typo, the keyword `suit` has been
replaced with `suite`, maintaining the backward
compatibility.
* Change logic for RTF_*_IF(condition, message)
functions, adding new functions RTF_*_IF_TRUE
and RTF_*_IF_FALSE, deprecating the old ones
Copy link
Member

Choose a reason for hiding this comment

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

The Markdown of this comment is not rendering correctly. I suspect you should make sure that the asterisk is not interpreted as the beginning of an italic sentence, for example using back-ticks: not good, *good*.

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore, do not forget the point at the end of the sentence.

*/
#define RTF_ASSERT_FAIL_IF(condition, message) \
RTF_TEST_REPORT("WARNING: RTF_ASSERT_FAIL_IF(condition, message) "\
"is deprecated and will be removed. Substitue with "\
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "Substitue --> substitute" . Furthermore, it is always nice to be kind to strangers: I would go for "Please substitute". : )

*/
#define RTF_ASSERT_ERROR_IF(condition, message)\
RTF_TEST_REPORT("WARNING: RTF_ASSERT_ERROR_IF(condition, message) "\
"is deprecated and will be removed. Substitue with "\
Copy link
Member

Choose a reason for hiding this comment

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

@aerydna
Copy link
Contributor

aerydna commented Oct 5, 2017

as @apaikan suggested in #67 let's check everything is fine in the bindings

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling c81b05e on damn1:rtf_testfaillogic into c75ba53 on robotology:devel.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling c81b05e on damn1:rtf_testfaillogic into c75ba53 on robotology:devel.

@damn1 damn1 force-pushed the rtf_testfaillogic branch 3 times, most recently from cd95923 to e4bd106 Compare October 6, 2017 08:59
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling c0d2216 on damn1:rtf_testfaillogic into 2084067 on robotology:devel.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling c0d2216 on damn1:rtf_testfaillogic into 2084067 on robotology:devel.

@aerydna aerydna merged commit 540ffaf into robotology:devel Oct 6, 2017
@aerydna
Copy link
Contributor

aerydna commented Oct 6, 2017

merged, thanks!

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

4 participants