Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9584 )
Change subject: IMPALA-5980: Upgrade to LLVM 5.0.1 ...................................................................... Patch Set 4: (5 comments) Minor comments on the clang-tidy fixes http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/rpc/impala-service-pool.cc@80 PS4, Line 80: ImpalaServicePool::Shutdown(); This was required because Shutdown() is virtual. However, I don't see why Shutdown() needs to be virtual since we don't subclass ImpalaServicePool as far as I can see. We should generally prefer non-virtual methods. I think Init() also is unnecessarily virtual. http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/transport/TSasl.cpp File be/src/transport/TSasl.cpp: http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/transport/TSasl.cpp@57 PS4, Line 57: (const char*) Switch this cast too? http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/transport/TSasl.cpp@72 PS4, Line 72: (const char* Switch this cast too? http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/transport/TSasl.cpp@242 PS4, Line 242: (const char*) Switch this cast and the one just below too? http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/udf/udf-test.cc File be/src/udf/udf-test.cc: http://gerrit.cloudera.org:8080/#/c/9584/4/be/src/udf/udf-test.cc@140 PS4, Line 140: // ptime t(*(date*)&time.date); is this conversion correct? Leftover comment. This looks right to me. -- To view, visit http://gerrit.cloudera.org:8080/9584 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0a15cb53feab89e7b35a56b67b3b30eb3e62c6b Gerrit-Change-Number: 9584 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 26 Mar 2018 18:59:51 +0000 Gerrit-HasComments: Yes
