Daniel Becker 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: (10 comments) I went through the rest. 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? 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(). 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(). 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? 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, shouldn't we do the same here? Usually when the value is not const we take it by pointer. 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? 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? 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: flags 'flags' is an unsigned int, no need to move it. Or would you like to move it for consistency? 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. 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@55 PS4, Line 55: int256_t I wonder if we ever have unaligned 'int256_t's. If we do, this might cause a problem if the compiler generates alignment-sensitive instructions here. I don't know if that's possible... I came across a similar thing in https://gerrit.cloudera.org/#/c/16134/, but it's possible that that commit also ensures this doesn't happen here. -- 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 14:32:21 +0000 Gerrit-HasComments: Yes
