This repository was archived by the owner on Aug 5, 2022. It is now read-only.
Add an option for removing Werror; have travis build in release config #310
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dawagner
force-pushed
the
optional-Werror-and-release-build
branch
from
7679571 to
f46758b
Compare
dawagner
changed the title
Contributor
|
👍 |
dawagner
force-pushed
the
optional-Werror-and-release-build
branch
2 times, most recently
from
7bd990a to
b3c6b53
Compare
dawagner
force-pushed
the
optional-Werror-and-release-build
branch
from
b3c6b53 to
a3cd3ad
Compare
Contributor
Author
|
@krocard ping? (cf. #310 (comment)) |
Contributor
There was a problem hiding this comment.
Lets pretend that binary returns the copy:
auto fData = utility::binaryCopy<float>(uiValue)I prefer to initialize variable on declaration.
Contributor
Author
There was a problem hiding this comment.
That's what I originally did. But consider line 177: I would find dangerous using an explicit template instantiation because, if the type of uiValue changes, the code might still compile but will not behave as intended.
Contributor
There was a problem hiding this comment.
What about decltype(uiValue) then ?
Contributor
Author
There was a problem hiding this comment.
I already considered this too. But I know from a previous try at another patch that Visual Studio 12 does not support using decltype on the variable being declared (which is the case in your suggestion). :(
dawagner
force-pushed
the
optional-Werror-and-release-build
branch
4 times, most recently
from
0b7842a to
012a605
Compare
Contributor
Author
Contributor
|
|
utility/test/utility.cpp
Outdated
Contributor
There was a problem hiding this comment.
WITH("Integer representation computed using http://babbage.cs.qc.cuny.edu/IEEE-754/") {
utility/test/utility.cpp
Outdated
Contributor
There was a problem hiding this comment.
Let me sumerize my proposition:
template <class V1, class V2>
checkBinaryEqual(V1 v1, V2 v2) {
CAPTURE(V1);
CAPTURE(V2);
CHECK(utility::binaryCopy<V1>(v2) == v1)
CHECK(utility::binaryCopy<V2>(v1) == v2)
}
SCENARIO("binaryCopy bit exactness") {
WITH("Integer representation computed using http://babbage.cs.qc.cuny.edu/IEEE-754/") {
THEN("Float should coded on 32bits. That assumption is made in the Parameter Framework.") {
REQUIRE(sizeof(float) == sizeof(uint32_t));
}
WHEN("Testing float <=> uint32_t conversion") {
checkBinaryEqual<float, int32_t>(1.23456f, 0x3f9e0610);
}
THEN("Double should coded on 64bits. That assumption is made in the Parameter Framework.") {
REQUIRE(sizeof(double) == sizeof(uint64_t));
}
WHEN("Testing double <=> uint64_t conversion") {
checkBinaryEqual<double, int64_t>(0x408edd3c0cb3420e, 987.65432109876)
}
WHEN("Testing int8_t <=> uint8_t conversion") {
checkBinaryEqual<int8_t, uint8_t>(-1, 0xff);
}
}To summarizer, try not to use comments in catch tests but instead use the SECTION & CO macro. This at the very least does not impact the readability and improves error messages.
dawagner
force-pushed
the
optional-Werror-and-release-build
branch
4 times, most recently
from
33c7fd3 to
8d12cdb
Compare
utility/test/utility.cpp
Outdated
Contributor
There was a problem hiding this comment.
Is did the CHECK(v1 == utility::binaryCopy<T1>(v2))); swap trick working ?
Contributor
Author
There was a problem hiding this comment.
nope :( I'm now trying with intermediate variables.
dawagner
force-pushed
the
optional-Werror-and-release-build
branch
from
8d12cdb to
21e45e2
Compare
We were using reinterpret_cast to convert from/to "raw"/float values but it breaks strict-aliasing rules. Instead, introduce a "binaryCopy" utility that uses a union to read a value from a type as if it was another type. Signed-off-by: David Wagner <david.wagner@intel.com>
…diff In release builds, with GCC, unused returned values of functions markes as "warn_unused_results" are enforced. We are calling `system`, that falls in this category. We now use the return value and take the opportunity to warn the user that we didn't manage to use "git diff" to display a rich diff. Signed-off-by: David Wagner <david.wagner@intel.com>
We have some warnings in Release configuration because of:
- unused arguments (some calls to `assert`, that are completly removed in
release mode, cause some arguments to be unused) in such a pattern:
void f(bool b)
{
assert(b);
}
- strict aliasing rules violation in floating point tests. Since this is only
test code, it is not considered critical.
Add an option (on by default) to add the -Werror flag. Have travis travis build
the Parameter Framework in "Release" configuration (it previously built in
"Debug" and "None" configurations) without the -Werror flag. Also, forcefully
remove the "/WX" flag on windows (equivalent to -Werror) because we are not
warning-free yet, even in debug mode.
Signed-off-by: David Wagner <david.wagner@intel.com>
Contributor
|
|
Contributor
Author
|
@krocard One last review, please? |
Contributor
|
|
krocard
added a commit
that referenced
this pull request
Nov 20, 2015
Add an option for removing Werror; have travis build in release config Fix some compilation errors in release configuration (strict aliasing violations) and add an option to enable/disable the -Werror flag. This is useful for integrators. Have travis build in release configuration instead of "None" configuration.
krocard
changed the title
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix some compilation errors in release configuration (strict aliasing violations) and add an option to enable/disable the -Werror flag. This is useful for integrators.
Have travis build in release configuration instead of "None" configuration.
CHECK(v1 == utility::binaryCopy<T1>(v2)));swap trick working ?I prefer to initialize variable on declaration.
uiValuechanges, the code might still compile but will not behave as intended.decltype(uiValue)then ?decltypeon the variable being declared (which is the case in your suggestion). :(I prefer to initialize variable on declaration.
I prefer to initialize variable on declaration.
I prefer to initialize variable on declaration.
Return by value ?
uiValue = utility::binaryCopy<decltype(uiValue)>(fValue);
CAPTURE(int1);
THEN("Float should coded on 32bits. That assumption is made in the Parameter Framework.") {
REQUIRE(sizeof(float) == sizeof(uint32_t));
}
I am not sure outX and inX variables or of any use. + that would make a nicer error message in case of failure.
THEN("Double should coded on 64bits. That assumption is made in the Parameter Framework.") {
REQUIRE(sizeof(double) == sizeof(uint64_t));
}
Maybe also test the reverse ? Consider:
Then replace all your
CHECKbycheckBinaryEqual. You will test each conversion direction for free !CHECK(utility::binaryCopy<int8_t>(0xff) == -1)
To summarizer, try not to use comments in catch tests but instead use the SECTION & CO macro. This at the very least does not impact the readability and improves error messages.