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

cppcheck: fix useStlAlgorithm #59

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Conversation

serval2412
Copy link
Contributor

src/libcmis/gdrive-document.cxx:76:13: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/gdrive-document.cxx:89:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/gdrive-document.cxx:95:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/json-utils.cxx:164:14: style: Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm]

src/libcmis/gdrive-document.cxx:76:13: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/gdrive-document.cxx:89:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/gdrive-document.cxx:95:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/json-utils.cxx:164:14: style: Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm]
@@ -69,35 +70,48 @@ string GDriveDocument::getDownloadUrl( string streamId )
if ( !streamId.empty( ) )
{
// Find the rendition associated with the streamId
for ( vector< RenditionPtr >::iterator it = renditions.begin( ) ;
it != renditions.end(); ++it )
std::vector<RenditionPtr>::iterator it = std::find_if(renditions.begin(), renditions.end(),
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, I don't think this improves the code in any way... If you really want to touch this, maybe using foreach loops would be better? That should make the code easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be sure, before I do anything wrong, do you prefer a foreach for the 4 changes or only for this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

For all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done.

src/libcmis/gdrive-document.cxx:76:13: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/gdrive-document.cxx:89:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/gdrive-document.cxx:95:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
src/libcmis/json-utils.cxx:164:14: style: Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm]
if ( (*it)->getMimeType( ).find( "opendocument") != string::npos )
return (*it)->getUrl( );
for (const auto& renditionPtr : renditions)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add extra braces unless needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done

@@ -69,35 +69,39 @@ string GDriveDocument::getDownloadUrl( string streamId )
if ( !streamId.empty( ) )
{
// Find the rendition associated with the streamId
for ( vector< RenditionPtr >::iterator it = renditions.begin( ) ;
it != renditions.end(); ++it )
for (const auto& renditionPtr : renditions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to follow the coding style of the surrounding code. (I don't like it very much myself, but, on my list, consistency is higher than personal preference.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure to understand here, do you mean I should use:
for (const auto& renditionPtr
: renditions)
?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that doesn't make much sense. What I mean is: try to use the same indentation, spaces around stuff, etc. like the surrounding code. E.g.,

if ( renditionPtr->getStreamId( ) == streamId )

instead of

if (renditionPtr->getStreamId() == streamId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I added spaces on all locations I had changed.
Hope it's ok now.

@@ -27,6 +27,7 @@
*/

#include "gdrive-document.hxx"
#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please use git commit --amend or git rebase --interactive to make "perfect" commits (followed by git push --force).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried too but sometimes the push is rejected on my local branch.
This time it seemed ok.

Copy link
Contributor

@dtardon dtardon 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 code-wise, just some style issues.

@serval2412
Copy link
Contributor Author

Is there anything I should change or is it ok?

Copy link
Contributor

@dtardon dtardon left a comment

Choose a reason for hiding this comment

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

LGTM

@dtardon dtardon merged commit 26f255c into tdf:master Feb 29, 2024
@serval2412 serval2412 deleted the useStlAlgorithm branch February 29, 2024 11:48
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.

2 participants