Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix Coverity COPY_PASTE_ERROR issues in acl_device_op_test.cpp #254

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

IlanTruanovsky
Copy link
Contributor

After taking a look at the code, it does not appear that these are copy paste errors.

I don't really like putting a comment to mark Coverity issues as false-positives, but I dislike obfuscating the code to resolve the error even more.

Fixes:

test/acl_device_op_test.cpp:987:3:
  Type: Copy-paste error (COPY_PASTE_ERROR)

test/acl_device_op_test.cpp:984:3:
  original: "op0->timestamp" looks like the original copy.
test/acl_device_op_test.cpp:987:3:
  copy_paste_error: "op0" in "op0->timestamp" looks like a copy-paste error.
test/acl_device_op_test.cpp:987:3:
  remediation: Should it say "op1" instead?

test/acl_device_op_test.cpp:906:3:
  Type: Copy-paste error (COPY_PASTE_ERROR)

test/acl_device_op_test.cpp:878:3:
  original: "op0->timestamp" looks like the original copy.
test/acl_device_op_test.cpp:906:3:
  copy_paste_error: "op0" in "op0->timestamp" looks like a copy-paste error.
test/acl_device_op_test.cpp:906:3:
  remediation: Should it say "op1" instead?

@IlanTruanovsky IlanTruanovsky self-assigned this Jan 26, 2023
@IlanTruanovsky IlanTruanovsky added the bug Something isn't working label Jan 26, 2023
@IlanTruanovsky IlanTruanovsky added this to the 2023.2 milestone Jan 26, 2023
@pcolberg
Copy link
Contributor

pcolberg commented Jan 26, 2023

I don't really like putting a comment to mark Coverity issues as false-positives, but I dislike obfuscating the code to resolve the error even more.

Yes, I agree with both. Maybe swapping the arguments works as suggested here? Besides swapping arguments, does reordering these lines have any effect? Taking a step back, how many more of these copy-paste issues are you seeing? Is there a common pattern to these issues that triggers Coverity? Are all of these false positives?

@IlanTruanovsky
Copy link
Contributor Author

I don't really like putting a comment to mark Coverity issues as false-positives, but I dislike obfuscating the code to resolve the error even more.

Yes, I agree with both. Maybe swapping the arguments works as suggested here? Besides swapping arguments, does reordering these lines have any effect? Taking a step back, how many more of these copy-paste issues are you seeing? Is there a common pattern to these issues that triggers Coverity?

There are only 5 of these Coverity issues in the runtime. I believe what triggers this is having similar expressions for some amount of lines, and then one line suddenly changing one (or possibly more) of the variables involved in those expressions, but utilizing the same overall structure. Reordering the lines does work, but I don't like that we have to do this just to avoid Coverity from complaining. It is still better than adding a comment, though.

Are all of these false positives?

It looks like 4/5 of them are. The only one that is likely not a false positive (but albeit a minor problem that doesn't effect our tests) is:

lib/pkg_editor/test/pkg_editor_test.cpp:336:3:
  Type: Copy-paste error (COPY_PASTE_ERROR)

lib/pkg_editor/test/pkg_editor_test.cpp:317:3:
  original: "strlen(file0)" looks like the original copy.
lib/pkg_editor/test/pkg_editor_test.cpp:336:3:
  copy_paste_error: "file0" in "strlen(file0)" looks like a copy-paste error.
lib/pkg_editor/test/pkg_editor_test.cpp:336:3:
  remediation: Should it say "file1" instead?

Taking a look at the code for that, it does look like it should be strlen(file1) instead of strlen(file0), but it doesn't matter since strlen(file0) == strlen(file1). I'll make a PR for this in the future when I finish with Coverity issues for our test dir.

@pcolberg
Copy link
Contributor

pcolberg commented Jan 27, 2023

There are only 5 of these Coverity issues in the runtime. I believe what triggers this is having similar expressions for some amount of lines, and then one line suddenly changing one (or possibly more) of the variables involved in those expressions, but utilizing the same overall structure. Reordering the lines does work, but I don't like that we have to do this just to avoid Coverity from complaining. It is still better than adding a comment, though.

Indeed, these tweaks are slightly annoying but still preferable over a all-caps marker that briefly draws and wastes the attention of the reader. 5 of these issues out of ~160 is not too bad. You can pick whichever tweak you think preserves the original intent better, reordering arguments or reordering lines.

Fixes:
```
test/acl_device_op_test.cpp:987:3:
  Type: Copy-paste error (COPY_PASTE_ERROR)

test/acl_device_op_test.cpp:984:3:
  original: "op0->timestamp" looks like the original copy.
test/acl_device_op_test.cpp:987:3:
  copy_paste_error: "op0" in "op0->timestamp" looks like a copy-paste error.
test/acl_device_op_test.cpp:987:3:
  remediation: Should it say "op1" instead?

test/acl_device_op_test.cpp:906:3:
  Type: Copy-paste error (COPY_PASTE_ERROR)

test/acl_device_op_test.cpp:878:3:
  original: "op0->timestamp" looks like the original copy.
test/acl_device_op_test.cpp:906:3:
  copy_paste_error: "op0" in "op0->timestamp" looks like a copy-paste error.
test/acl_device_op_test.cpp:906:3:
  remediation: Should it say "op1" instead?
```
@IlanTruanovsky
Copy link
Contributor Author

@pcolberg I've removed the comment. This should still make Coverity happy.

@IlanTruanovsky IlanTruanovsky changed the title Mark COPY_PASTE_ERROR as false positives Fix Coverity COPY_PASTE_ERROR issues in acl_device_op_test.cpp Jan 27, 2023
@pcolberg pcolberg merged commit 67e23e1 into intel:main Jan 27, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants