Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23057 )
Change subject: [compile] Fix compiler warnings about -Wrange-loop-construct ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/23057/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23057/2//COMMIT_MSG@15 PS2, Line 15: and binding a reference to them is potentially unsafe : and triggers compiler warnings. Nope, I don't think that's true. The temporary object has the loop scope, so there isn't anything unsafe. The point behind the compiler's warning is to let the user know that a temporary variable is created anyway, even if the reference used for iteration. But there isn't anything dramatic or unsafe in those warnings. http://gerrit.cloudera.org:8080/#/c/23057/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/23057/2/src/kudu/tools/kudu-tool-test.cc@7819 PS2, Line 7819: * nit here and below: the asterisk should stick to the type per style guide convention: 'const auto* const ...' http://gerrit.cloudera.org:8080/#/c/23057/2/src/kudu/tools/kudu-tool-test.cc@9173 PS2, Line 9173: string daemon_type(daemon_type_str); I'm not a big fun of string copying. Could we get away with using std::string_view instead of creating temporary std::string here anyway and other places like this? for (const string_view : { ... }) { ... } In addition, you'll probably need to add a constructor for Substitute arguments: diff --git a/src/kudu/gutil/strings/substitute.h b/src/kudu/gutil/strings/substi tute.h index cb2c81864..c5678ccc4 100644 --- a/src/kudu/gutil/strings/substitute.h +++ b/src/kudu/gutil/strings/substitute.h @@ -2,6 +2,7 @@ #include <cstring> #include <string> +#include <string_view> #include "kudu/gutil/strings/numbers.h" #include "kudu/gutil/strings/stringpiece.h" @@ -74,6 +75,8 @@ class SubstituteArg { : text_(value), size_(value == NULL ? 0 : strlen(text_)) {} inline SubstituteArg(const std::string& value) // NOLINT(runtime/explicit) : text_(value.data()), size_(value.size()) {} + inline SubstituteArg(const std::string_view& value) // NOLINT(runtime/explic it) + : text_(value.data()), size_(value.size()) {} inline SubstituteArg(const StringPiece& value) // NOLINT(runtime/explicit) : text_(value.data()), size_(value.size()) {} -- To view, visit http://gerrit.cloudera.org:8080/23057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32a9a043e7aade2c2c879472c13cbf2641daa7f3 Gerrit-Change-Number: 23057 Gerrit-PatchSet: 2 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 27 Jun 2025 06:04:28 +0000 Gerrit-HasComments: Yes
