think-cell / think-cell-library

think-cell core library
https://www.think-cell.com/en/career/devblog/overview
Boost Software License 1.0
407 stars 52 forks source link

clean compile with compiler flags /WX /W4 #17

Closed frederich closed 7 years ago

frederich commented 7 years ago

Dear Tobias,

I use /WX and /W4 in my projects. This commit tidies up some minor compile issues. The boost::ignore_unused might be replaced with a better VERIFYEQUAL mock-up. In general, I think we should add proper default implementations for VERIFYEQUAL, _ASSERTPRINT instead of empty mock-ups.

Best Jens

frederich commented 7 years ago

Tobias, any comment?

treh commented 7 years ago

Jens, Could you resolve the conflicts arising from the latest update?

frederich commented 7 years ago

Sure

frederich commented 7 years ago

Tobias, please take a look.

I'd prefer the PR #19 over #17, because there are less foul boost::ignore_unused calls.

frederich commented 7 years ago

Ping, all conflicts resolved.

treh commented 7 years ago

Woudn't it be better to just exclude the unit tests (*.t.cpp) from the release build?

schoedl commented 7 years ago

We probably want to test Release, too.

-- Dr. Arno Sch�dl | schoedl@think-cell.com Technical Director

We are looking for C++ Developers: https://www.think-cell.com/career


think-cell Software GmbH https://www.think-cell.comhttps://www.think-cell.com/ Chausseestr. 8/E phone / fax +49 30 666473-10 / -19 10115 Berlin, Germany US phone / fax +1 800 891 8091 / +1 212 504 3039 Amtsgericht Berlin-Charlottenburg, HRB 180042 | European Union VAT Id DE308385481 Directors: Dr. Markus Hannebauer, Dr. Arno Sch�dl On 2 Jan 2017, at 16:32, Tobias Reh notifications@github.com<mailto:notifications@github.com> wrote:

Woudn't it be better to just exclude the unit tests (*.t.cpp) from the release build?

� You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/think-cell/range/pull/17#issuecomment-269979684, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AD_nCwXm-qANpHneUoMMwQguFkN9QO94ks5rOQpIgaJpZM4K9w4t.

frederich commented 7 years ago

PR accepted or shall I rework VERIFYEQUAL to avoid boot::ignore_unused?

treh commented 7 years ago

Release build should either have a meaningful _ASSERT or skip the unit tests. Only silencing the warnings (by boost::unused_arg or by static_cast does not matter) is not useful.

frederich commented 7 years ago

Personally, I prefer to keep asserts in Release, thus meaningful _ASSERTs. I'll send a proposal.

frederich commented 7 years ago

BTW, union_range_test fails in Release mode

Running Unit test 'union_range_tests'...
Fail: Ranges differ:
Expected: [2, 16975008, 3, 4, 4, 32, 47, ]
Is      : [1, 1, 1, 2, 3, 3, 4, 4, 4, 5, ]
Running Unit test 'union_range_generator' ...
Running Unit test 'sub_range_with_tranform' ...
Running Unit test 'Unique_Ranges' ...
Fail: Ranges differ:
Expected: [1, ]
Is      : [-20094001, 15727176, 19268344, 0, 15727124, 19193958, 19153555, 19153555, 13225984, 19013152, -20094941, 15727192, 19153311, 19272000, 19272508, -20094865, 19153555, 19153555, 13225984, 0, 19153555, 2376339, 15727140, 0, 15727268, 19170256, -16415617, 0, 15727212, 2008638148, 13225984, 2008638112, -2010548387, 15727284, 2010451929, 13225984, -2012604734, 0, 0, 13225984, 0, 0, 0, 0, -2012604734, 15727224, 0, 15727292, 2010539952, -12883098, ]
treh commented 7 years ago

What's your test setup? Note that VS 2015 Update 1,2,3 are buggy and have code generation errors. Please use either VS 2015 without Update or VS 2017 RC.

frederich commented 7 years ago

Ah, good to know, thanks! My current setup is VS 2015 Update 3. I'll upgrade soon.