Don't use C style casts by laudrup · Pull Request #363 · USCiLab/cereal

laudrup · 2016-11-23T00:40:59Z

Change C style casts to C++ static casts.

This makes the code compile with the -Wold-style-cast warning enabled

Cereal uses fairly clean modern C++ style, but unfortunately a few headers use C style casts meaning they cannot be used without disabling warnings about these casts, using #pragmas or similar.

This shouldn't introduce any functional changes.

Change C style casts to C++ static casts. This makes the code compile with the -Wold-style-cast warning enabled

erichkeane · 2016-11-23T07:30:50Z

      void bind(std::false_type) const
 	    {
-		    instantiate_polymorphic_binding((T*) 0, 0, Tag{}, adl_tag{});
+                   instantiate_polymorphic_binding(static_cast<T*>(0), 0, Tag{}, adl_tag{});


Almost definitely should be nullptr.

erichkeane · 2016-11-23T07:33:00Z

        if (i ==4) {
          for (i = 0; i <4; i++)
-            char_array_4[i] = (unsigned char) chars.find( char_array_4[i] );
+            char_array_4[i] = static_cast<unsigned char>(chars.find( char_array_4[i] ));


Is there a good reason for char_array* to be unsigned char, particularly when the string is a std::string. alternatively, chars should probably be a fixed array, right?

erichkeane · 2016-11-23T07:33:48Z

    assert( d1->x == 4 && d1->y == 3 );
    iar( d2 );
-    assert( ((Derived*)d2.get())->x == 5 && ((Derived*)d2.get())->y == 4 );
+    assert( static_cast<Derived*>(d2.get())->x == 5 && static_cast<Derived*>(d2.get())->y == 4 );


Dynamic cast + a check?

erichkeane · 2016-11-23T07:35:11Z

 void load( Archive & ar, Bla & b )
 {
-  ar( (int&)b );
+  ar( static_cast<int&>(b) );


erichkeane · 2016-11-23T07:35:23Z

 void save( Archive & ar, Bla const & b )
 {
-  ar( (const int &)b );
+  ar( static_cast<const int &>(b) );


I don't totally see what is happening here, but is the cast totally necessary? Should this cast be to std::underlying_type?

I think a lot of the code in the sandbox files was temporary for debugging development, especially early on in cereal. Most tests of note are put into actual test cases now.

erichkeane · 2016-11-23T07:36:04Z

    BOOST_CHECK_EQUAL(i_uint64 , o_uint64);
    BOOST_CHECK_EQUAL(i_int64  , o_int64);
-    BOOST_CHECK_CLOSE(i_float  , o_float,  (float)1e-5);
+    BOOST_CHECK_CLOSE(i_float  , o_float,  static_cast<float>(1e-5));


erichkeane · 2016-11-23T07:36:42Z

    BOOST_CHECK_EQUAL(i_uint64 , o_uint64);              \
    BOOST_CHECK_EQUAL(i_int64  , o_int64);               \
-    if( !std::isnan(i_float) && !std::isnan(o_float) ) BOOST_CHECK_CLOSE(i_float  , o_float,  (float)1e-5); \
+    if( !std::isnan(i_float) && !std::isnan(o_float) ) BOOST_CHECK_CLOSE(i_float  , o_float,  static_cast<float>(1e-5)); \


erichkeane · 2016-11-23T07:37:01Z

    BOOST_CHECK(i_isplv.x == o_isplv.x);

-    BOOST_CHECK_EQUAL(((SpecializedMSplitPolymorphic*)i_shared_ispl.get())->x, ((SpecializedMSplitPolymorphic*)o_shared_ispl.get())->x);
+    BOOST_CHECK_EQUAL(static_cast<SpecializedMSplitPolymorphic*>(i_shared_ispl.get())->x, static_cast<SpecializedMSplitPolymorphic*>(o_shared_ispl.get())->x);


Dynamic + check?

erichkeane · 2016-11-23T07:37:15Z

-    BOOST_CHECK_EQUAL(*((PolyDerived*)i_shared.get()), *((PolyDerived*)i_locked.get()));
-    BOOST_CHECK_EQUAL(*((PolyDerived*)i_locked.get()), *((PolyDerived*)o_locked.get()));
-    BOOST_CHECK_EQUAL(*((PolyDerived*)i_unique.get()), *((PolyDerived*)o_unique.get()));
+    BOOST_CHECK_EQUAL(*static_cast<PolyDerived*>(i_shared.get()), *static_cast<PolyDerived*>(o_shared.get()));


Dynamic + check for all?

laudrup · 2016-11-23T10:05:59Z

@erichkeane

Very reasonable comments.

All I did in this commit was to "blindly" change the C casts to static_casts. I didn't want to risk changing any functionality without a better understanding of the code.

I cannot see that this commit introduces any of the issues you have commented on. Am I missing something?

Thanks.

kklouzal · 2016-11-23T10:30:20Z

@laudrup You're missing the fact that he did exactly as you wanted and changed all the C-Style Casts into C++ static_cast's 👍 👍

laudrup · 2016-11-23T12:02:57Z

@kklouzal

One of us is confused. I was the one who changed the C-style casts to C++ static_casts. @erichkeane came with the very good points about how to improve it. :-)

erichkeane · 2016-11-23T16:43:28Z

I'm not maintainer, so this obviously isn't up to me :)

Just thought @AzothAmmo might find my suggestions useful. A vast majority of these are in unittest/sandbox, so they don't really matter that much anyway

AzothAmmo · 2016-11-23T17:42:15Z

I'll work these in taking @erichkeane's comments into account.

erichkeane · 2016-11-23T17:47:05Z

👍

laudrup · 2016-11-23T17:48:18Z

@AzothAmmo Cool. Thanks a lot. Let me know if I can help in any way, but as I wrote I'm not too familiar with this code so I'd rather avoid getting in the way.

see #363

AzothAmmo · 2016-11-28T19:54:22Z

Modified with most of the suggestions of @erichkeane, didn't bother with checks on the dynamic casts since they'll fail out the unit tests either way if they don't work.

laudrup · 2016-11-28T20:36:14Z

Nice!

Thanks a lot for this.

commit aa891a4 Author: Shane Grant <[email protected]> Date: Mon Jun 26 13:42:08 2017 -0700 Resolves USCiLab#414 commit fcef0da Author: Shane Grant <[email protected]> Date: Fri May 5 11:37:12 2017 -0700 Fix shadowing issue for USCiLab#401, recent osx compile issue re: USCiLab#354 commit 950aca4 Merge: ad90557 35a36af Author: Shane Grant <[email protected]> Date: Fri May 5 11:09:34 2017 -0700 Merge branch 'hoensr-xml-no-size-attributes' into develop commit 35a36af Author: Shane Grant <[email protected]> Date: Fri May 5 10:54:25 2017 -0700 Standardize interface for options (xml) see USCiLab#401 commit ad90557 Merge: f031131 2ab15f7 Author: Shane Grant <[email protected]> Date: Wed May 3 11:39:02 2017 -0700 Merge branch 'develop' of github.com:USCiLab/cereal into develop commit f031131 Author: Shane Grant <[email protected]> Date: Wed May 3 11:38:35 2017 -0700 modifications for g47 and comment out memory intensive testing commit c4dcc8d Merge: 68f56ee 676d329 Author: Shane Grant <[email protected]> Date: Wed May 3 11:11:08 2017 -0700 Merge branch 'issue_354' into develop still needs more work but this is an improvement for now see USCiLab#354 commit 52b03d5 Author: Robin Hoens <[email protected]> Date: Mon Apr 10 17:13:39 2017 +0200 Add option to turn off the size=dynamic attributes of lists commit 2ab15f7 Merge: 68f56ee 7723503 Author: Shane Grant <[email protected]> Date: Tue Apr 11 16:17:51 2017 -0700 Merge pull request USCiLab#397 from albertziegenhagel/issue_396 Add remove_reference to determine whether template argument to BinaryData is const commit 7723503 Author: Albert Ziegenhagel <[email protected]> Date: Tue Apr 11 10:32:55 2017 +0200 Add remove_reference to determine whether template argument to BinaryData is const commit 68f56ee Author: Shane Grant <[email protected]> Date: Mon Apr 10 11:22:38 2017 -0700 Remove undefined behavior, see USCiLab#390 commit 676d329 Author: Shane Grant <[email protected]> Date: Wed Feb 22 14:43:31 2017 -0800 adventures in microoptimization USCiLab#354 commit 546fd9b Author: Shane Grant <[email protected]> Date: Sun Feb 19 16:25:32 2017 -0800 tinkering on USCiLab#354 commit 8b8f581 Author: Shane Grant <[email protected]> Date: Wed Feb 15 13:39:42 2017 -0800 Fix macro for double comparison in unit test relates USCiLab#338 commit 51cbda5 Merge: 70c4420 e38d6fe Author: Shane Grant <[email protected]> Date: Sun Feb 12 14:06:58 2017 -0800 Merge branch 'develop' for release 1.2.2 commit e38d6fe Author: Shane Grant <[email protected]> Date: Sun Feb 12 13:46:25 2017 -0800 fix update doc script commit 70c4420 Author: Shane Grant <[email protected]> Date: Sun Feb 12 00:25:30 2017 -0800 Update README.md add appveyor badge commit 2590f21 Author: Shane Grant <[email protected]> Date: Sat Feb 11 23:47:00 2017 -0800 Properly use multimap for lookup in poly casting relates USCiLab#356, still need final testing on MSVC commit a917374 Author: Shane Grant <[email protected]> Date: Wed Feb 8 10:47:02 2017 -0800 size_type now specified by macro CEREAL_SIZE_TYPE resolves USCiLab#379 commit ee17db5 Merge: b827b95 f577fc4 Author: Shane Grant <[email protected]> Date: Mon Feb 6 22:51:29 2017 -0800 Merge branch 'develop' of github.com:USCiLab/cereal into develop commit b827b95 Author: Shane Grant <[email protected]> Date: Mon Feb 6 22:50:56 2017 -0800 Fixes need for special MSVC case, see USCiLab#373 commit f577fc4 Author: Shane Grant <[email protected]> Date: Sun Feb 5 22:13:08 2017 -0800 Turn on warnings as errors for MSVC, warning level to 3 Can't do level 4 warnings yet - need to make an upstream change to doctest commit fb6606d Author: Shane Grant <[email protected]> Date: Sun Feb 5 18:13:37 2017 -0800 Do not build coverage or valgrind for MSVC commit a2d5a15 Merge: 4a92e29 e4d543d Author: Shane Grant <[email protected]> Date: Fri Jan 27 10:29:51 2017 -0800 Merge branch 'tusharpm-develop' into develop see USCiLab#373 Still need to address why windows needed a modifcation to polymorphic test to compile commit e4d543d Author: Tushar Maheshwari <[email protected]> Date: Thu Jan 26 16:53:47 2017 +0530 Fix merge issues commit 2261fee Author: Tushar Maheshwari <[email protected]> Date: Sun Dec 18 17:32:04 2016 +0530 Pull requests to not increment build numbers commit 4ff4db8 Author: Tushar Maheshwari <[email protected]> Date: Sun Dec 11 20:03:02 2016 +0530 boost new version commit 655696a Author: Tushar Maheshwari <[email protected]> Date: Sun Dec 11 15:42:00 2016 +0530 AppVeyor integration commit df44243 Author: Tushar Maheshwari <[email protected]> Date: Sun Dec 11 01:01:10 2016 +0530 Enable cross-platform portability test CMake fix 32-bit executable with generator Win64 commit 0a908bc Author: Tushar Maheshwari <[email protected]> Date: Sat Dec 10 17:27:28 2016 +0530 Make tests pass with Windows commit 4a92e29 Author: Shane Grant <[email protected]> Date: Wed Jan 25 11:04:24 2017 -0800 no longer need boost test in travis commit a8e9963 Merge: 75e50ee 1d67d44 Author: Shane Grant <[email protected]> Date: Wed Jan 25 10:56:38 2017 -0800 Merge branch 'develop_doctest' into develop see USCiLab#139 commit 75e50ee Author: Shane Grant <[email protected]> Date: Mon Nov 28 13:48:46 2016 -0800 remove old code from sandbox relates USCiLab#363 commit 507f97d Author: Shane Grant <[email protected]> Date: Mon Nov 28 11:53:56 2016 -0800 add -Wold-style-casts see USCiLab#363 commit f69ad7c Author: Shane Grant <[email protected]> Date: Mon Nov 28 11:50:23 2016 -0800 tab to space commit d21b0c0 Merge: 29829c1 e63f08f Author: Shane Grant <[email protected]> Date: Mon Nov 28 11:47:53 2016 -0800 Merge branch 'laudrup-master' into develop see USCiLab#363 commit e63f08f Author: Shane Grant <[email protected]> Date: Mon Nov 28 11:47:24 2016 -0800 minor adjustments to casts, see comments on USCiLab#363 commit 29829c1 Merge: 9978e0c ad92746 Author: Shane Grant <[email protected]> Date: Mon Nov 28 10:41:27 2016 -0800 Merge branch 'FlexCoreLib-doxygen_access_from_extern' into develop see USCiLab#365 commit ad92746 Author: Shane Grant <[email protected]> Date: Mon Nov 28 10:41:02 2016 -0800 comment out obsolete doxygen commit 9978e0c Merge: 1edc5c6 6e71766 Author: Shane Grant <[email protected]> Date: Mon Nov 28 10:37:17 2016 -0800 Merge pull request USCiLab#366 from headupinclouds/pr.is_loading.is_saving Add is_loading and is_saving values to cereal::{Input,Output}Archive commit 1edc5c6 Merge: 72d7936 6086234 Author: Shane Grant <[email protected]> Date: Mon Nov 28 10:35:24 2016 -0800 Merge pull request USCiLab#367 from tusharpm/develop Travis configuration updates commit 6086234 Author: Tushar Maheshwari <[email protected]> Date: Sun Nov 27 13:57:26 2016 +0530 Travis configuration updates - Switch to trusty container-based infrastructure for linux systems - Add a job to test with clang compiler (skips portability test) - Add a job to test on macOS with Xcode8 commit 6e71766 Author: David Hirvonen <[email protected]> Date: Sat Nov 26 12:31:56 2016 -0500 Remove spurious character commit 9376ca6 Author: Kasper Laudrup <[email protected]> Date: Wed Nov 23 01:34:53 2016 +0100 Don't use C style casts Change C style casts to C++ static casts. This makes the code compile with the -Wold-style-cast warning enabled commit 1d67d44 Author: Shane Grant <[email protected]> Date: Fri Nov 4 16:07:00 2016 -0700 Removed boost_test as a requirement -Now build 32 bit unit tests if portability testing is enabled -No more boost+clang issues over std::string abi see USCiLab#139, USCiLab#123 commit 66528b6 Author: Shane Grant <[email protected]> Date: Fri Nov 4 15:51:57 2016 -0700 last? conversions for USCiLab#139 commit 978b3b5 Author: Shane Grant <[email protected]> Date: Fri Nov 4 12:11:51 2016 -0700 typo fix in comment USCiLab#139 commit 13ae560 Author: Shane Grant <[email protected]> Date: Fri Nov 4 12:10:24 2016 -0700 more USCiLab#139 commit 07818f4 Author: Shane Grant <[email protected]> Date: Fri Nov 4 12:00:16 2016 -0700 more USCiLab#139 commit b5e500d Author: Shane Grant <[email protected]> Date: Wed Nov 2 16:32:35 2016 -0700 more USCiLab#139 commit a6e59d7 Author: Shane Grant <[email protected]> Date: Wed Nov 2 13:38:33 2016 -0700 more for USCiLab#139 commit cd46374 Author: Shane Grant <[email protected]> Date: Wed Nov 2 12:20:22 2016 -0700 more converted for USCiLab#139 commit 38e1548 Author: Shane Grant <[email protected]> Date: Tue Nov 1 11:59:40 2016 -0700 headers USCiLab#139 commit 15c7339 Author: Shane Grant <[email protected]> Date: Tue Nov 1 11:59:02 2016 -0700 more conversions USCiLab#139 commit 928cd36 Author: Shane Grant <[email protected]> Date: Tue Nov 1 11:41:28 2016 -0700 more conversions to CHECK_EQ see USCiLab#139 commit 0a262ec Author: Shane Grant <[email protected]> Date: Tue Nov 1 11:36:22 2016 -0700 More tests split, switch to CHECk_EQ over CHECK see USCiLab#139 commit 671999e Author: Shane Grant <[email protected]> Date: Fri Oct 28 14:28:37 2016 -0700 Initial progress on removing boost test and moving to doctest to better support modules relates USCiLab#123 commit a066808 Author: Caspar Kielwein <[email protected]> Date: Sun Oct 2 16:09:04 2016 +0200 Add cereal.doxytags as tagfile to allow external projects to link to cereal documentation. commit 3fb59db Author: Caspar Kielwein <[email protected]> Date: Sun Oct 2 15:53:28 2016 +0200 Build doxygen documentation with separate CMakeLists. Extracted CMakelists.txt to doc subdirectory. Changed paths in doxyfile.in and CMakeLists accordingly. added doc as subdirectory in main CMakeLists.txt.

Don't use C style casts

9376ca6

Change C style casts to C++ static casts. This makes the code compile with the -Wold-style-cast warning enabled

erichkeane reviewed Nov 23, 2016

View reviewed changes

AzothAmmo added the enhancement label Nov 23, 2016

AzothAmmo added this to the v1.2.3 milestone Nov 23, 2016

AzothAmmo modified the milestones: v1.2.2, v1.2.3 Nov 28, 2016

AzothAmmo added a commit that referenced this pull request Nov 28, 2016

minor adjustments to casts, see comments on #363

e63f08f

AzothAmmo added a commit that referenced this pull request Nov 28, 2016

Merge branch 'laudrup-master' into develop

d21b0c0

see #363

AzothAmmo added a commit that referenced this pull request Nov 28, 2016

add -Wold-style-casts see #363

507f97d

AzothAmmo closed this Nov 28, 2016

AzothAmmo added a commit that referenced this pull request Nov 28, 2016

remove old code from sandbox relates #363

75e50ee

erichkeane mentioned this pull request Nov 29, 2016

Using C++11 static_cast instead of C-like casts. #369

Closed

Uh oh!

Conversation

laudrup commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laudrup commented Nov 23, 2016

Uh oh!

kklouzal commented Nov 23, 2016

Uh oh!

laudrup commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane commented Nov 23, 2016

Uh oh!

AzothAmmo commented Nov 23, 2016

Uh oh!

erichkeane commented Nov 23, 2016

Uh oh!

laudrup commented Nov 23, 2016

Uh oh!

AzothAmmo commented Nov 28, 2016

Uh oh!

laudrup commented Nov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laudrup commented Nov 23, 2016 •

edited

Loading