Skip to content

Cppcheck cleanup - #11397

Merged
Nir-Az merged 11 commits into
realsenseai:developmentfrom
dmipx:cppcheck-cleanup
Feb 19, 2023
Merged

Cppcheck cleanup#11397
Nir-Az merged 11 commits into
realsenseai:developmentfrom
dmipx:cppcheck-cleanup

Conversation

@dmipx

@dmipx dmipx commented Feb 5, 2023 •
edited by Nir-Az
Loading

Copy link
Copy Markdown

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

@dmipx
dmipx force-pushed the cppcheck-cleanup branch 3 times, most recently from 4502d27 to 8a362bf Compare February 5, 2023 16:07
@Nir-Az
Nir-Az requested a review from OhadMeir February 5, 2023 16:19
Nir-Az
Nir-Az reviewed Feb 5, 2023
View reviewed changes
Comment thread third-party/stb_image.h Outdated
@dmipx
dmipx force-pushed the cppcheck-cleanup branch 3 times, most recently from 025341b to decb545 Compare February 5, 2023 18:22
OhadMeir
OhadMeir reviewed Feb 5, 2023
View reviewed changes
Comment thread common/calibration-model.cpp Outdated
OhadMeir
OhadMeir reviewed Feb 5, 2023
View reviewed changes
Comment thread src/ds/ds5/ds5-auto-calibration.cpp Outdated
OhadMeir
OhadMeir reviewed Feb 5, 2023
View reviewed changes
Comment thread third-party/rsutils/include/rsutils/concurrency/concurrency.h Outdated
OhadMeir
OhadMeir reviewed Feb 5, 2023
View reviewed changes
Comment thread third-party/stb_image.h Outdated
OhadMeir
OhadMeir reviewed Feb 5, 2023
View reviewed changes
Comment thread third-party/stb_image.h Outdated
k &= stbi__bmask[n];
j->code_bits -= n;
return k + (stbi__jbias[n] & ~sgn);
return k + (stbi__jbias[n] & (sgn - 1));

@OhadMeir OhadMeir Feb 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice trick using -1 instead of ~

@dmipx dmipx Feb 6, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

OhadMeir
OhadMeir reviewed Feb 5, 2023
View reviewed changes

@OhadMeir OhadMeir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@dmipx

dmipx commented Feb 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

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'.`

@dmipx
dmipx requested a review from remibettan February 6, 2023 08:18
remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread common/calibration-model.cpp Outdated
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();

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread common/calibration-model.cpp Outdated
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 remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread common/model-views.cpp
std::string& error_message)
{
option_model option;
option_model option = {};

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is it better than before?

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

According to Ohad research looks like this zeros all native members

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/ds/ds5/ds5-auto-calibration.cpp Outdated

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 });

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please use static_cast here

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/ds/ds5/ds5-auto-calibration.cpp Outdated
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 });

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

static_cast

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/ds/ds5/ds5-auto-calibration.cpp Outdated
{
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 remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sc

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/ds/ds5/ds5-auto-calibration.cpp Outdated
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;

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sc

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/l500/l500-color.cpp
remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/l500/l500-options.cpp Outdated
}

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

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sc

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

remibettan
remibettan reviewed Feb 6, 2023
View reviewed changes
Comment thread src/l500/l500-options.cpp Outdated
_type,
l500_command::get_default,
(int)query_sensor_mode( *_resolution ) },
(uint32_t)query_sensor_mode( *_resolution ) },

@remibettan remibettan Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sc

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

Dmitry Perchanov added 2 commits February 16, 2023 14:55
Dmitry Perchanov @Nir-Az
stb_image: Avoid shift of signed values in extend_receive
c0938aa 
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 realsenseai#1061.
nothings/stb@0260135

Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
Dmitry Perchanov @Nir-Az
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>
Dmitry Perchanov and others added 5 commits February 16, 2023 14:55
Dmitry Perchanov @Nir-Az
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>
Dmitry Perchanov @Nir-Az
hw-monitor: parameter registers are uint32_t.
8c8ab84 
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>
Dmitry Perchanov @Nir-Az
common: calibration-model: reset table pointers.
6d5212c 
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>
Dmitry Perchanov @Nir-Az
third-party: stb_image upgrade to v2.28
65da31d 
Signed-off-by: Dmitry Perchanov <dmitry.perchanov@intel.com>
@Nir-Az
fix cr comments
59a285e
OhadMeir
OhadMeir reviewed Feb 16, 2023
View reviewed changes
Comment thread common/calibration-model.cpp Outdated
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());

@OhadMeir OhadMeir Feb 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please run formatting on changed lines

@Nir-Az Nir-Az Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done

OhadMeir
OhadMeir approved these changes Feb 16, 2023
View reviewed changes

@OhadMeir OhadMeir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

remibettan
remibettan reviewed Feb 19, 2023
View reviewed changes
Comment thread src/l500/l500-options.cpp
// 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 ) } );

@remibettan remibettan Feb 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

remibettan
remibettan approved these changes Feb 19, 2023
View reviewed changes

@remibettan remibettan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Nir-Az
Nir-Az merged commit c0a5dbb into realsenseai:development Feb 19, 2023
@dmipx
dmipx deleted the cppcheck-cleanup branch February 26, 2023 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nir-Az Nir-Az Nir-Az left review comments
@remibettan remibettan remibettan approved these changes
@OhadMeir OhadMeir OhadMeir approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dmipx @remibettan @Nir-Az @OhadMeir