permission: address coverity warning#50215
Closed
mhdawson wants to merge 1 commit intonodejs:mainfrom
Closed
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Member
Author
|
Coverity has a new set of warnings. Seeing what we think about fixing those by starting with this one. If there is a possitive response I'd plan to fix in larget baches. The warning was: 212 if (is_wildcard_node || is_last_char) {
213 std::string node_path = path.substr(parent_node_prefix_len, i);
CID 329770 (#1 of 1): COPY_INSTEAD_OF_MOVE (COPY_INSTEAD_OF_MOVE)
1. copy_constructor_call: node_path is passed-by-value as parameter to CreateChild when it could be moved instead.
Use std::move(node_path) instead of node_path.
214 current_node = current_node->CreateChild(node_path); |
RafaelGSS
approved these changes
Oct 17, 2023
Member
Which compiler are you using? I didn't get these warnings building on macOS |
benjamingr
approved these changes
Oct 17, 2023
Member
Author
|
@RafaelGSS they are not from the compiler from but our Coverity static analysis - https://scan.coverity.com/projects/node-js |
bnoordhuis
reviewed
Oct 18, 2023
src/permission/fs_permission.cc
Outdated
| if (is_wildcard_node || is_last_char) { | ||
| std::string node_path = path.substr(parent_node_prefix_len, i); | ||
| current_node = current_node->CreateChild(node_path); | ||
| current_node = current_node->CreateChild(std::move(node_path)); |
Member
There was a problem hiding this comment.
This doesn't really change anything (except maybe shut up coverity) because CreateChild takes a value, not a reference. What you want instead is this:
diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h
index 244e95727a..80374a08c7 100644
--- a/src/permission/fs_permission.h
+++ b/src/permission/fs_permission.h
@@ -31,7 +31,7 @@ class FSPermission final : public PermissionBase {
Node() : wildcard_child(nullptr), is_leaf(false) {}
- Node* CreateChild(std::string prefix) {
+ Node* CreateChild(const std::string& prefix) {
if (prefix.empty() && !is_leaf) {
is_leaf = true;
return this;Or Node* CreateChild(std::string&& prefix) { + std::move(...). Look at other call sites to determine which is most appropriate.
Member
Author
|
@bnoordhuis thanks, updated |
Member
|
@mhdawson can you rebase? |
The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions. Signed-off-by: Michael Dawson <midawson@redhat.com>
Member
Author
|
@RafaelGSS rebased on squashed commits |
github-actions
bot
removed
the
request-ci
Collaborator
Collaborator
marco-ippolito
approved these changes
Oct 26, 2023
19 tasks
mhdawson
added a commit
that referenced
this pull request
Oct 27, 2023
The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: #50215 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Member
Author
|
Landed in 3c1b4b3 |
This was referenced Oct 28, 2023
alexfernandez
pushed a commit
to alexfernandez/node
that referenced
this pull request
Nov 1, 2023
The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: nodejs#50215 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
targos
pushed a commit
that referenced
this pull request
Nov 11, 2023
The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: #50215 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
UlisesGascon
pushed a commit
that referenced
this pull request
Dec 11, 2023
The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: #50215 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The latest version of coverity has suggestions on how to improve formance. Address one of these suggestions.