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: (3 comments) I will continue reviewing this tonight. Looks like it is better to review using clion, in case IDE can point out something that vim can't. 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) { nit: There are only few callsites calling this method. Maybe we can consider making 'name' mandatory instead of optional? That way, the name.empty() branch is not needed anymore, and callsites might realize they can reuse 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 This is not const string& because TryStripPrefixString mutate auth_header? http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@664 PS16, Line 664: const string& auth_header this is unused? -- 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: Wed, 30 Oct 2024 22:30:44 +0000 Gerrit-HasComments: Yes
