Cppcheck cleanup #11397

dmipx · 2023-02-05T13:21:02Z

Fix CppCheck errors:

E uninitvar                      common/model-views.cpp+554                         Uninitialized variables: option.unset_value, option.have_unset_value, option.read_only, option.edit_mode
E uninitvar                      src/l500/l500-color.cpp+265                        Uninitialized variable: intrinsics.model
E integerOverflow                src/l500/l500-depth.cpp+322                        Signed integer overflow for expression 'baseline_address+4'.
E integerOverflow                src/l500/l500-device.cpp+321                       Signed integer overflow for expression 'ivcam2::REGISTER_CLOCK_0+4'.
E invalidContainer               common/calibration-model.cpp+347                   Using pointer to member variable '_calibration' that may be invalid.
E invalidContainer               common/calibration-model.cpp+347                   Using pointer to member variable '_original' that may be invalid.
E danglingLifetime               third-party/rsutils/include/rsutils/concurrency/concurrency.h+54 Non-local variable '_queue' will use object that points to local variable 'q'.
E danglingLifetime               third-party/rsutils/include/rsutils/concurrency/concurrency.h+55 Non-local variable '_queue' will use object that points to local variable 'q'.
E uninitStructMember             src/l500/l500-color.cpp+265                        Uninitialized struct member: intrinsics.model

third-party/stb_image.h

common/calibration-model.cpp

src/ds/ds5/ds5-auto-calibration.cpp

third-party/rsutils/include/rsutils/concurrency/concurrency.h

third-party/stb_image.h

OhadMeir · 2023-02-05T20:57:32Z

third-party/stb_image.h

   k = stbi_lrot(j->code_buffer, n);
   STBI_ASSERT(n >= 0 && n < (int) (sizeof(stbi__bmask)/sizeof(*stbi__bmask)));
   j->code_buffer = k & ~stbi__bmask[n];
   k &= stbi__bmask[n];
   j->code_bits -= n;
-   return k + (stbi__jbias[n] & ~sgn);
+   return k + (stbi__jbias[n] & (sgn - 1));


Nice trick using -1 instead of ~

this is fixed issue in original source
nothings/stb@0260135

OhadMeir

Nice work.
Getting rid of the cppcheck errors is helpful and the code logic is not always easy to follow...

dmipx · 2023-02-06T07:56:25Z

We left with

E danglingLifetime third-party/rsutils/include/rsutils/concurrency/concurrency.h+54 Non-local variable '_queue' will use object that points to local variable 'q'.

E danglingLifetime third-party/rsutils/include/rsutils/concurrency/concurrency.h+55 Non-local variable '_queue' will use object that points to local variable 'q'.`

remibettan · 2023-02-06T08:21:02Z

common/calibration-model.cpp

@@ -299,6 +299,8 @@ void calibration_model::update(ux_window& window, std::string& error_message)
                    dev.as<rs2::auto_calibrated_device>().reset_to_factory_calibration();
                    _calibration = dev.as<rs2::auto_calibrated_device>().get_calibration_table();
                    _original = _calibration;
+                    table = (librealsense::ds::coefficients_table*)_calibration.data();


Please avoid the c-style casting, and use cpp style (static_cast or reinterpret_cast)

remibettan · 2023-02-06T08:21:12Z

common/calibration-model.cpp

@@ -444,6 +446,7 @@ void calibration_model::update(ux_window& window, std::string& error_message)
                    dev.as<rs2::auto_calibrated_device>().set_calibration_table(_calibration);
                    dev.as<rs2::auto_calibrated_device>().write_calibration();
                    _original = _calibration;
+                    orig_table = (librealsense::ds::coefficients_table*)_original.data();


remibettan · 2023-02-06T08:21:35Z

common/model-views.cpp

@@ -523,7 +523,7 @@ namespace rs2
        bool* options_invalidated,
        std::string& error_message)
    {
-        option_model option;
+        option_model option = {};


Why is it better than before?

According to Ohad research looks like this zeros all native members

remibettan · 2023-02-06T08:22:18Z

src/ds/ds5/ds5-auto-calibration.cpp

@@ -602,7 +602,7 @@ namespace librealsense
            }

            if (host_assistance == host_assistance_type::no_assistance || host_assistance == host_assistance_type::assistance_start)
-                _hw_monitor->send(command{ ds::AUTO_CALIB, focal_length_calib_begin, fl_step_count, fy_scan_range, p4 });
+                _hw_monitor->send(command{ ds::AUTO_CALIB, focal_length_calib_begin, (uint32_t)fl_step_count, (uint32_t)fy_scan_range, p4 });


please use static_cast here

remibettan · 2023-02-06T08:22:30Z

src/ds/ds5/ds5-auto-calibration.cpp

@@ -717,7 +717,7 @@ namespace librealsense
            // Begin auto-calibration
            if (host_assistance == host_assistance_type::no_assistance || host_assistance == host_assistance_type::assistance_start)
            {
-                _hw_monitor->send(command{ ds::AUTO_CALIB, py_rx_plus_fl_calib_begin, speed_fl, 0, p4 });
+                _hw_monitor->send(command{ ds::AUTO_CALIB, py_rx_plus_fl_calib_begin, (uint32_t)speed_fl, 0, p4 });


static_cast

remibettan · 2023-02-06T08:22:37Z

src/ds/ds5/ds5-auto-calibration.cpp

@@ -898,7 +898,7 @@ namespace librealsense
        if (depth > 0)
        {
            LOG_OCC_WARN("run_tare_calibration interactive control (2) with parameters: depth = " << depth);
-            _hw_monitor->send(command{ ds::AUTO_CALIB, interactive_scan_control, 2, depth });
+            _hw_monitor->send(command{ ds::AUTO_CALIB, interactive_scan_control, 2, (uint32_t)depth });


remibettan · 2023-02-06T08:22:43Z

src/ds/ds5/ds5-auto-calibration.cpp

@@ -917,7 +917,7 @@ namespace librealsense
                LOG_DEBUG("run_tare_calibration with parameters: speed = " << speed << " average_step_count = " << average_step_count << " step_count = " << step_count << " accuracy = " << accuracy << " scan_parameter = " << scan_parameter << " data_sampling = " << data_sampling);
                check_tare_params(speed, scan_parameter, data_sampling, average_step_count, step_count, accuracy);

-                auto param2 = (int)ground_truth_mm * 100;
+                auto param2 = (uint32_t)ground_truth_mm * 100;


src/l500/l500-color.cpp

remibettan · 2023-02-06T08:24:42Z

src/l500/l500-options.cpp

@@ -38,7 +38,7 @@ namespace librealsense
                    "Alternate IR cannot be enabled with IR Reflectivity" );
        }

-        _hw_monitor->send(command{ AMCSET, _type, (int)value });
+        _hw_monitor->send(command{ AMCSET, (uint32_t)_type, (uint32_t)value });


remibettan · 2023-02-06T08:24:52Z

src/l500/l500-options.cpp

@@ -138,7 +138,7 @@ namespace librealsense
        auto res = _hw_monitor->send( command{ AMCGET,
                                               _type,
                                               l500_command::get_default,
-                                               (int)query_sensor_mode( *_resolution ) },
+                                               (uint32_t)query_sensor_mode( *_resolution ) },


E shiftTooManyBitsSigned third-party/stb_image.h+1647 Shifting signed 32-bit value by 31 bits is undefined behaviour Use an equivalent formulation that has sgn=0 or 1, not 0 or -1. This avoids right-shifting signed values, at least in this place. Fixes issue IntelRealSense#1061. nothings/stb@0260135 Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

E uninitvar src/l500/l500-color.cpp+265 Uninitialized variable: intrinsics.model Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

E uninitvar common/model-views.cpp+554 Uninitialized variables: option.unset_value, option.have_unset_value, option.read_only, option.edit_mode Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

cppcheck: E integerOverflow src/l500/l500-depth.cpp+322 Signed integer overflow for expression 'baseline_address+4'. E integerOverflow src/l500/l500-device.cpp+321 Signed integer overflow for expression 'ivcam2::REGISTER_CLOCK_0+4'. l500: l500-options: msvc fix for narrowing conversion Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

E invalidContainer common/calibration-model.cpp+350 Using pointer to member variable '_calibration' that may be invalid. E invalidContainer common/calibration-model.cpp+350 Using pointer to member variable '_original' that may be invalid. Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

OhadMeir · 2023-02-16T12:59:16Z

common/calibration-model.cpp

-                    table = (librealsense::ds::coefficients_table*)_calibration.data();
-                    orig_table = (librealsense::ds::coefficients_table*)_original.data();
+                    table = reinterpret_cast<librealsense::ds::coefficients_table*>(_calibration.data());
+                    orig_table = reinterpret_cast<librealsense::ds::coefficients_table*> (_original.data());


Please run formatting on changed lines

OhadMeir

LGTM

remibettan · 2023-02-19T10:17:41Z

src/l500/l500-options.cpp

@@ -176,7 +177,7 @@ namespace librealsense
        //     4. Read the current value
        //     5. Restore the current value
        auto current = query_current( query_sensor_mode( *_resolution ) );
-        _hw_monitor->send( command{ AMCSET, _type, -1 } );
+        _hw_monitor->send( command{ AMCSET, _type, static_cast< uint32_t >( -1 ) } );


This will result to UINT_MAX as input (not -1) - is it ok here?

remibettan

LGTM

dmipx force-pushed the cppcheck-cleanup branch 3 times, most recently from 4502d27 to 8a362bf Compare February 5, 2023 16:07

Nir-Az requested a review from OhadMeir February 5, 2023 16:19

Nir-Az reviewed Feb 5, 2023

View reviewed changes

third-party/stb_image.h Outdated Show resolved Hide resolved

dmipx force-pushed the cppcheck-cleanup branch 3 times, most recently from 025341b to decb545 Compare February 5, 2023 18:22

OhadMeir reviewed Feb 5, 2023

View reviewed changes

common/calibration-model.cpp Outdated Show resolved Hide resolved

OhadMeir reviewed Feb 5, 2023

View reviewed changes

src/ds/ds5/ds5-auto-calibration.cpp Outdated Show resolved Hide resolved

OhadMeir reviewed Feb 5, 2023

View reviewed changes

third-party/rsutils/include/rsutils/concurrency/concurrency.h Outdated Show resolved Hide resolved

OhadMeir reviewed Feb 5, 2023

View reviewed changes

third-party/stb_image.h Outdated Show resolved Hide resolved

OhadMeir reviewed Feb 5, 2023

View reviewed changes

dmipx force-pushed the cppcheck-cleanup branch from decb545 to be54117 Compare February 6, 2023 07:21

dmipx requested a review from remibettan February 6, 2023 08:18

remibettan reviewed Feb 6, 2023

View reviewed changes

src/l500/l500-color.cpp Show resolved Hide resolved

remibettan reviewed Feb 6, 2023

View reviewed changes

dmipx force-pushed the cppcheck-cleanup branch from d6f1184 to f7b7913 Compare February 6, 2023 08:49

dmipx added 2 commits February 16, 2023 14:55

cppcheck: l500-color

e063a65

E uninitvar src/l500/l500-color.cpp+265 Uninitialized variable: intrinsics.model Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

dmipx and others added 5 commits February 16, 2023 14:55

common:model-views: Uninitialized variables

0058d4f

E uninitvar common/model-views.cpp+554 Uninitialized variables: option.unset_value, option.have_unset_value, option.read_only, option.edit_mode Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

third-party: stb_image upgrade to v2.28

65da31d

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>

fix cr comments

59a285e

OhadMeir reviewed Feb 16, 2023

View reviewed changes

change to static_cast and undo stb changes

de67617

Nir-Az force-pushed the cppcheck-cleanup branch from 1cac4f7 to de67617 Compare February 16, 2023 13:49

Nir-Az added 2 commits February 16, 2023 16:35

protect dispatcher object

13c3f86

upload new cppcheck logs

3f42b7c

Nir-Az requested review from remibettan and OhadMeir February 16, 2023 15:38

Trigger Build

8a4dd55

OhadMeir approved these changes Feb 16, 2023

View reviewed changes

remibettan reviewed Feb 19, 2023

View reviewed changes

remibettan approved these changes Feb 19, 2023

View reviewed changes

Nir-Az merged commit c0a5dbb into IntelRealSense:development Feb 19, 2023

dmipx deleted the cppcheck-cleanup branch February 26, 2023 07:59

Cppcheck cleanup #11397

Cppcheck cleanup #11397

Uh oh!

Conversation

dmipx commented Feb 5, 2023 • edited by Nir-Az Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OhadMeir left a comment

Choose a reason for hiding this comment

Uh oh!

dmipx commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

OhadMeir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remibettan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmipx commented Feb 5, 2023 •

edited by Nir-Az

Loading

dmipx commented Feb 6, 2023 •

edited

Loading