Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param ...................................................................... Patch Set 16: (10 comments) In several placed, clion gave this warning over move syntax: Unqualified call to 'std::move' Maybe all new 'move' in this patch should be addressed as 'std::move' to be safe. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc File be/src/benchmarks/parquet-delta-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc@236 PS16, Line 236: move clion says this should be std::move, here and below. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/codegen-anyval-read-write-info.h File be/src/codegen/codegen-anyval-read-write-info.h: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/codegen-anyval-read-write-info.h@173 PS16, Line 173: // Creates a PHI node that will have the value 'non_null_value' if the incoming block is : // 'non_null_block_' and the value 'null_value' if it is 'null_block_'. : llvm::PHINode* CodegenNullPhiNode( : llvm::Value* non_null_value, llvm::Value* null_value, const std::string& name = ""); : : // Creates a PHI node the value of which tells whether it was reached from the non-null : // or the null path, i.e. whether this CodegenAnyValReadWriteInfo is null. : llvm::PHINode* CodegenIsNullPhiNode(const std::string& name = ""); 'name' can be made mandatory here as well. "val_ptr_phi" in filter-context.cc can be a constant string. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@629 PS16, Line 629: string > Right. I mention this change in the commit message. Ack http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@664 PS16, Line 664: const string& auth_header > It's used at line 670. Ack http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc File be/src/rpc/thrift-thread.cc: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc@86 PS16, Line 86: move clion says this should be std::move. Maybe because there is no "using namespace std;" here? http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h File be/src/transport/TSaslServerTransport.h: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@112 PS16, Line 112: : /** : * Construct a new TSaslTrasnport, passing in the components of the definition. : */ : TSaslServerTransport(std::string mechanism, : std::string protocol, : std::string serverName, : std::string realm, : unsigned flags, : std::map<std::string, std::string> props, : std::vector<struct sasl_callback> callbacks, : std::shared_ptr<TTransport> transport); I think this constructor is unused now. Please double check. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@152 PS16, Line 152: /** : * Create a new Factor for a server definition. : * Provides a single definition for the server, others may be added. : */ : Factory(const std::string& mechanism, const std::string& protocol, : const std::string& serverName, const std::string& realm, : unsigned flags, std::map<std::string, std::string> props, : std::vector<struct sasl_callback> callbacks) : : TTransportFactory() { : addServerDefinition(mechanism, protocol, serverName, realm, flags, : move(props), move(callbacks)); : } I think this constructor is unused now. Please double check. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h@69 PS16, Line 69: const std::string& separator = "," All callers pass '\n' as separator. Perhaps this can be replaced with PrintIdsInMultiLine() without separator parameter. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/parquet-reader.cc File be/src/util/parquet-reader.cc: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/parquet-reader.cc@62 PS16, Line 62: return tproto_factory.getProtocol(move(mem)); : } else { : TBinaryProtocolFactoryT<TMemoryBuffer> tproto_factory; : return tproto_factory.getProtocol(move(mem)); clion says this should be std::move. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/periodic-counter-updater.cc File be/src/util/periodic-counter-updater.cc: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/periodic-counter-updater.cc@65 PS16, Line 65: move clion says this should be std::move. -- 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: 16 Gerrit-Owner: Michael Smith <[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-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Thu, 31 Oct 2024 02:33:16 +0000 Gerrit-HasComments: Yes
