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 16: (10 comments) 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 > The compiler doesn't have a problem with it, but I can go ahead and be spec We explicitly specify 'using std::move' in common/names.h. I'd rather stick with the unornamented one in .cc files. 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. https://issues.apache.org/jira/browse/IMPALA-13502 http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc@697 PS16, Line 697: llvm::PHINode* LlvmCodeGen::CreateBinaryPhiNode(LlvmBuilder* builder, llvm::Value* value1, : llvm::Value* value2, llvm::BasicBlock* incoming_block1, : llvm::BasicBlock* incoming_block2, const string& name) { > This patch is pretty huge, and focused on the performance checks. I'll crea https://issues.apache.org/jira/browse/IMPALA-13502 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 > It includes common/names.h. Maybe it's the _GLIBCXX_UTILITY guard that's ca Covered by common/names.h. http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h File be/src/runtime/outbound-row-batch.h: http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h@38 PS16, Line 38: OutboundRowBatch(const std::shared_ptr<CharMemTrackerAllocator>& allocator) > This patch is pretty huge, and focused on the performance checks. I'll crea https://issues.apache.org/jira/browse/IMPALA-13502 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); > This patch is pretty huge, and focused on the performance checks. I'll crea https://issues.apache.org/jira/browse/IMPALA-13502 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)); : } > This patch is pretty huge, and focused on the performance checks. I'll crea https://issues.apache.org/jira/browse/IMPALA-13502 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 Print https://issues.apache.org/jira/browse/IMPALA-13502 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. Covered by common/names.h. 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. Updated this just to be consistent with the others below. SampleFunction is a boost::function, so it also implicitly matches boost::move and we need to be explicit. We could definitely also do a pass and convert lots of boost classes to std classes, like boost::function -> std::function. That's for another ticket. -- 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 18:03:55 +0000 Gerrit-HasComments: Yes
