1626787 - ClearKeyDecryptionManager.cpp: the 'empty' method should be used to check for emptiness instead of 'size'

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Sylvestre Ledru [:Sylvestre]]

+++ This bug was initially created as a clone of Bug #1626786 +++

Filling as a good first bug to learn workflows.

aMetadata.mIV.size() == 0
should use .empty() instead

https://searchfox.org/mozilla-central/source/media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp#261

As the change is trivial, it is just to learn how to contribute to Firefox.

Found by http://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html

Tutorial to contribute:
https://firefox-source-docs.mozilla.org/tools/docs/contribute/how_to_contribute_firefox.html

Please don't ask for the bug to be assigned. It will be automatically assigned to the first patch.

Comment 1

[Jayati Shrivastava]

Attachment: Bug 1626787 - Use .empty() instead of .size() to check emptiness. r=Sylvestre

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Jayati, as said in phabricator, please leave trivial good first bug to others. you have been working on several already.

Comment 3

[Jayati Shrivastava]

Really Sorry about that. I shall leave these for other developers.

Comment 4

[Omri Sarig]

Attachment: Bug 1626787 - ClearKeyDecryptionManager.cpp check emptiness by empty instead of by size. r?Sylvestre

Emptiness of values should be checked using the empty method, and not
by comparing the size to 0. Fix such case in the code of
ClearKeyDecryptionManager.cpp module.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a760ec2bb3a
ClearKeyDecryptionManager.cpp check emptiness by empty instead of by size. r=sylvestre

Comment 6

[Omri Sarig]

Hey Sylvestre,

I saw that you approved the fix to the bug, thanks for that!

I saw that the reviewbot added a comment that the file is uncovered, so it is either dead code (and should be removed) or code that is not tested (and should be tested). I saw that this code is used at least in the same module, but I don't understand the big picture good enough to understand if this code is used in other places as well.

If I understand correctly, this issue should not fix the tests as well, but in case the code is used, a new issue should be opened about adding tests to the code.
This is my first issue here, and I don't feel comfortable enough to open issues here yet, so I didn't do it.
In case a new issue really should be open for the tests, I would love to be assigned to it and help write the tests for this code (even though I don't know it yet). However, I might need help or guidance from someone who understands the code and the testing in order to do it well.

If I understood correctly the comment of the Pulsebot above, the code was inserted into the main repository, so thanks for that again. Will the issue be closed by itself in some time, or is there anything more that needs to be done in order to close it?

Thanks,
Omri

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Nothing to do anymore. The bug will be closed when the patch will move from autoland => mozilla-central
About the code coverage, don't bother. :) We might not run the test for this code as it is pretty complex

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0a760ec2bb3a