Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param ...................................................................... Patch Set 4: (12 comments) > Also, running an ASAN test job would be a good idea because this patch > touches ownership in many places. I ran an ASAN job and it passed. I'll want to do another one after addressing all review comments. http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc File be/src/rpc/thrift-thread.cc: http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc@70 PS2, Line 70: // Passing runnable in to this method (rather than reading from this->runnable()) > I've tried it with a small test file and I think you're right, std::bind se Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/runtime/outbound-row-batch.h File be/src/runtime/outbound-row-batch.h: http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/runtime/outbound-row-batch.h@38 PS4, Line 38: allocator > Shouldn't we take it by raw pointer or reference? Yeah. I started using that pattern partway through, so missed some. http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h@1502 PS4, Line 1502: session > Could take by raw pointer, as in for example SetupResultsCacheing(). Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h@1510 PS4, Line 1510: session > Could take by raw pointer, as in for example SetupResultsCacheing(). Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc@889 PS4, Line 889: query_record > Couldn't this be a const ref? Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc@1464 PS4, Line 1464: & > We take 'session_state' in ImpalaServer::RegisterQuery() by pointer, should Right, that's a convention I still don't quite get but will follow. http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h@106 PS4, Line 106: std::string&, std::string& > Can't these be const refs? Yeah. Implementations got confusing, but const& should be fine for them too. http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h@111 PS4, Line 111: std::string& > Can't this be a const ref? Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp File be/src/transport/TSaslServerTransport.cpp: http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp@59 PS4, Line 59: move(props), move(callbacks) > 'props' and 'callbacks' are taken by reference. Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp@59 PS4, Line 59: flags > 'flags' is an unsigned int, no need to move it. Or would you like to move i Done http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h File be/src/util/decimal-util.h: http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@49 PS4, Line 49: static T SafeMultiply(T a, T b, bool may_overflow) { > How much does it matter that the SafeMultiply here and the int256_t version I think SafeMultiply below isn't actually a template specialization. http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@55 PS4, Line 55: int256_t > I wonder if we ever have unaligned 'int256_t's. If we do, this might cause I'm not sure. This call is flagged because the operators for int256_t all take values by const&, so we're making an unnecessary copy. -- To view, visit http://gerrit.cloudera.org:8080/20494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003 Gerrit-Change-Number: 20494 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Fri, 22 Sep 2023 19:11:25 +0000 Gerrit-HasComments: Yes
