Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addressing static analysis errors #1640

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Addressing static analysis errors #1640

merged 3 commits into from
Jun 27, 2024

Conversation

timmiesmith
Copy link
Contributor

@timmiesmith timmiesmith commented Jun 25, 2024

Scanning oneDPL code with a static analysis tool identified the two versions of a complex comparator in the SIMD unseq backend. This PR removes the user-defined copy constructors since they are explicit versions of the default copy constructor to address the errors from the static analysis tool.

In addition the scans also identified two functions used in the implementation of uninitialized memory algorithms that were tagged with the noexcept keyword. In the case of the uninitialized memory functors new may be called, in which case an exception could be thrown.

mmichel11
mmichel11 previously approved these changes Jun 25, 2024
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM

danhoeflinger
danhoeflinger previously approved these changes Jun 25, 2024
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM

adamfidel
adamfidel previously approved these changes Jun 25, 2024
Copy link
Contributor

@adamfidel adamfidel left a comment

Choose a reason for hiding this comment

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

Looks good, but I'm curious what were the errors from the static analysis tool? If the user-defined copy constructor is identical to the default copy constructor, what is the error? Or is the error that it the user-defined copy constructor is unnecessary?

@timmiesmith timmiesmith dismissed stale reviews from adamfidel, danhoeflinger, and mmichel11 via ae4745d June 25, 2024 21:29
@timmiesmith timmiesmith changed the title Removing user-defined copy ctors to resolve static analysis errors Addressing static analysis errors Jun 25, 2024
@@ -104,7 +104,7 @@ __for_each_n_it_serial(_ForwardIterator __first, _Size __n, _Function __f)
//------------------------------------------------------------------------
template <class _ForwardIterator, class _Function>
void
__brick_walk1(_ForwardIterator __first, _ForwardIterator __last, _Function __f, /*vector=*/::std::false_type) noexcept
Copy link
Contributor

Choose a reason for hiding this comment

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

From the CI, it looks like there is a forward declaration you also need to change.

@timmiesmith
Copy link
Contributor Author

Looks good, but I'm curious what were the errors from the static analysis tool? If the user-defined copy constructor is identical to the default copy constructor, what is the error? Or is the error that it the user-defined copy constructor is unnecessary?

Static analysis identified it as a violation of the Rule of Three, basically seeing the user-defined copy constructor and expecting to see corresponding user-defined assignment operator and destructor.

@timmiesmith
Copy link
Contributor Author

CI issues have been resolved.

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@adamfidel adamfidel left a comment

Choose a reason for hiding this comment

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

LGTM

@timmiesmith timmiesmith merged commit 850fa69 into main Jun 27, 2024
20 checks passed
@timmiesmith timmiesmith deleted the static-analysis-fixes branch June 27, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants